From ff8a053d5ddf154cd52c3e21ac24619dbbee0dc7 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 15 May 2017 16:13:36 -0700
Subject: [PATCH 01/16] Fix Git over HTTP spec
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* The spec has 7 failures at this point
* Specify rendered error messages
* Render the GitAccess message rather than “Access denied”
* Render the Not Found message provided by GitAccess, instead of a custom one
* Expect GitAccess to check the config for whether Git-over-HTTP pull or push is disabled, rather than doing it in the controller
* Add more thorough testing for authentication
* Dried up a lot of tests
* Fixed some broken tests
---
 lib/gitlab/checks/change_access.rb |  36 +-
 lib/gitlab/git_access.rb           |  11 +-
 lib/gitlab/git_access_wiki.rb      |   6 +-
 spec/requests/git_http_spec.rb     | 646 +++++++++++++++++------------
 spec/support/git_http_helpers.rb   |  19 +-
 5 files changed, 434 insertions(+), 284 deletions(-)

diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index c984eb20606..4b6f8abd61d 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -1,6 +1,20 @@
 module Gitlab
   module Checks
     class ChangeAccess
+      ERROR_MESSAGES = {
+        push_code: 'You are not allowed to push code to this project.',
+        delete_default_branch: 'The default branch of a project cannot be deleted.',
+        force_push_protected_branch: 'You are not allowed to force push code to a protected branch on this project.',
+        non_master_delete_protected_branch: 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.',
+        non_web_delete_protected_branch: 'You can only delete protected branches using the web interface.',
+        merge_protected_branch: 'You are not allowed to merge code into protected branches on this project.',
+        push_protected_branch: 'You are not allowed to push code to protected branches on this project.',
+        change_existing_tags: 'You are not allowed to change existing tags on this project.',
+        update_protected_tag: 'Protected tags cannot be updated.',
+        delete_protected_tag: 'Protected tags cannot be deleted.',
+        create_protected_tag: 'You are not allowed to create this tag as it is protected.'
+      }.freeze
+
       attr_reader :user_access, :project, :skip_authorization, :protocol
 
       def initialize(
@@ -32,7 +46,7 @@ module Gitlab
 
       def push_checks
         if user_access.cannot_do_action?(:push_code)
-          "You are not allowed to push code to this project."
+          ERROR_MESSAGES[:push_code]
         end
       end
 
@@ -40,7 +54,7 @@ module Gitlab
         return unless @branch_name
 
         if deletion? && @branch_name == project.default_branch
-          return "The default branch of a project cannot be deleted."
+          return ERROR_MESSAGES[:delete_default_branch]
         end
 
         protected_branch_checks
@@ -50,7 +64,7 @@ module Gitlab
         return unless ProtectedBranch.protected?(project, @branch_name)
 
         if forced_push?
-          return "You are not allowed to force push code to a protected branch on this project."
+          return ERROR_MESSAGES[:force_push_protected_branch]
         end
 
         if deletion?
@@ -62,22 +76,22 @@ module Gitlab
 
       def protected_branch_deletion_checks
         unless user_access.can_delete_branch?(@branch_name)
-          return 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.'
+          return ERROR_MESSAGES[:non_master_delete_protected_branch]
         end
 
         unless protocol == 'web'
-          'You can only delete protected branches using the web interface.'
+          ERROR_MESSAGES[:non_web_delete_protected_branch]
         end
       end
 
       def protected_branch_push_checks
         if matching_merge_request?
           unless user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
-            "You are not allowed to merge code into protected branches on this project."
+            ERROR_MESSAGES[:merge_protected_branch]
           end
         else
           unless user_access.can_push_to_branch?(@branch_name)
-            "You are not allowed to push code to protected branches on this project."
+            ERROR_MESSAGES[:push_protected_branch]
           end
         end
       end
@@ -86,7 +100,7 @@ module Gitlab
         return unless @tag_name
 
         if tag_exists? && user_access.cannot_do_action?(:admin_project)
-          return "You are not allowed to change existing tags on this project."
+          return ERROR_MESSAGES[:change_existing_tags]
         end
 
         protected_tag_checks
@@ -95,11 +109,11 @@ module Gitlab
       def protected_tag_checks
         return unless ProtectedTag.protected?(project, @tag_name)
 
-        return "Protected tags cannot be updated." if update?
-        return "Protected tags cannot be deleted." if deletion?
+        return ERROR_MESSAGES[:update_protected_tag] if update?
+        return ERROR_MESSAGES[:delete_protected_tag] if deletion?
 
         unless user_access.can_create_tag?(@tag_name)
-          return "You are not allowed to create this tag as it is protected."
+          return ERROR_MESSAGES[:create_protected_tag]
         end
       end
 
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 99724db8da2..591f68cd415 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -9,7 +9,10 @@ module Gitlab
       download: 'You are not allowed to download code from this project.',
       deploy_key_upload:
         'This deploy key does not have write access to this project.',
-      no_repo: 'A repository for this project does not exist yet.'
+      no_repo: 'A repository for this project does not exist yet.',
+      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."
     }.freeze
 
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze
@@ -73,19 +76,19 @@ module Gitlab
       return if deploy_key?
 
       if user && !user_access.allowed?
-        raise UnauthorizedError, "Your account has been blocked."
+        raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
       end
     end
 
     def check_project_accessibility!
       if project.blank? || !can_read_project?
-        raise UnauthorizedError, 'The project you were looking for could not be found.'
+        raise UnauthorizedError, ERROR_MESSAGES[:project_not_found]
       end
     end
 
     def check_command_existence!(cmd)
       unless ALL_COMMANDS.include?(cmd)
-        raise UnauthorizedError, "The command you're trying to execute is not allowed."
+        raise UnauthorizedError, ERROR_MESSAGES[:command_not_allowed]
       end
     end
 
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 67eaa5e088d..dccafd2cae8 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -1,5 +1,9 @@
 module Gitlab
   class GitAccessWiki < GitAccess
+    ERROR_MESSAGES = {
+      write_to_wiki: "You are not allowed to write to this project's wiki."
+    }.freeze
+
     def guest_can_download_code?
       Guest.can?(:download_wiki_code, project)
     end
@@ -12,7 +16,7 @@ module Gitlab
       if user_access.can_do_action?(:create_wiki)
         build_status_object(true)
       else
-        build_status_object(false, "You are not allowed to write to this project's wiki.")
+        build_status_object(false, ERROR_MESSAGES[:write_to_wiki])
       end
     end
   end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 6ca3ef18fe6..080e2f12cd7 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -5,76 +5,217 @@ describe 'Git HTTP requests', lib: true do
   include WorkhorseHelpers
   include UserActivitiesHelpers
 
-  it "gives WWW-Authenticate hints" do
-    clone_get('doesnt/exist.git')
+  shared_examples 'pulls require Basic HTTP Authentication' do
+    context "when no credentials are provided" do
+      it "responds to downloads with status 401 Unauthorized (no project existence information leak)" do
+        download(path) do |response|
+          expect(response).to have_http_status(:unauthorized)
+          expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+        end
+      end
+    end
 
-    expect(response.header['WWW-Authenticate']).to start_with('Basic ')
-  end
+    context "when only username is provided" do
+      it "responds to downloads with status 401 Unauthorized" do
+        download(path, user: user.username) do |response|
+          expect(response).to have_http_status(:unauthorized)
+          expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+        end
+      end
+    end
 
-  describe "User with no identities" do
-    let(:user)    { create(:user) }
-    let(:project) { create(:project, :repository, path: 'project.git-project') }
-
-    context "when the project doesn't exist" do
-      context "when no authentication is provided" do
-        it "responds with status 401 (no project existence information leak)" do
-          download('doesnt/exist.git') do |response|
-            expect(response).to have_http_status(401)
+    context "when username and password are provided" do
+      context "when authentication fails" do
+        it "responds to downloads with status 401 Unauthorized" do
+          download(path, user: user.username, password: "wrong-password") do |response|
+            expect(response).to have_http_status(:unauthorized)
+            expect(response.header['WWW-Authenticate']).to start_with('Basic ')
           end
         end
       end
 
-      context "when username and password are provided" do
-        context "when authentication fails" do
-          it "responds with status 401" do
-            download('doesnt/exist.git', user: user.username, password: "nope") do |response|
-              expect(response).to have_http_status(401)
-            end
+      context "when authentication succeeds" do
+        it "does not respond to downloads with status 401 Unauthorized" do
+          download(path, user: user.username, password: user.password) do |response|
+            expect(response).not_to have_http_status(:unauthorized)
+            expect(response.header['WWW-Authenticate']).to be_nil
           end
         end
+      end
+    end
+  end
 
-        context "when authentication succeeds" do
-          it "responds with status 404" do
-            download('/doesnt/exist.git', user: user.username, password: user.password) do |response|
-              expect(response).to have_http_status(404)
-            end
+  shared_examples 'pushes require Basic HTTP Authentication' do
+    context "when no credentials are provided" do
+      it "responds to uploads with status 401 Unauthorized (no project existence information leak)" do
+        upload(path) do |response|
+          expect(response).to have_http_status(:unauthorized)
+          expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+        end
+      end
+    end
+
+    context "when only username is provided" do
+      it "responds to uploads with status 401 Unauthorized" do
+        upload(path, user: user.username) do |response|
+          expect(response).to have_http_status(:unauthorized)
+          expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+        end
+      end
+    end
+
+    context "when username and password are provided" do
+      context "when authentication fails" do
+        it "responds to uploads with status 401 Unauthorized" do
+          upload(path, user: user.username, password: "wrong-password") do |response|
+            expect(response).to have_http_status(:unauthorized)
+            expect(response.header['WWW-Authenticate']).to start_with('Basic ')
+          end
+        end
+      end
+
+      context "when authentication succeeds" do
+        it "does not respond to uploads with status 401 Unauthorized" do
+          upload(path, user: user.username, password: user.password) do |response|
+            expect(response).not_to have_http_status(:unauthorized)
+            expect(response.header['WWW-Authenticate']).to be_nil
+          end
+        end
+      end
+    end
+  end
+
+  shared_examples_for 'pulls are allowed' do
+    it do
+      download(path, env) do |response|
+        expect(response).to have_http_status(:ok)
+        expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+      end
+    end
+  end
+
+  shared_examples_for 'pushes are allowed' do
+    it do
+      upload(path, env) do |response|
+        expect(response).to have_http_status(:ok)
+        expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+      end
+    end
+  end
+
+  describe "User with no identities" do
+    let(:user) { create(:user) }
+
+    context "when the project doesn't exist" do
+      let(:path) { 'doesnt/exist.git' }
+
+      it_behaves_like 'pulls require Basic HTTP Authentication'
+      it_behaves_like 'pushes require Basic HTTP Authentication'
+
+      context 'when authenticated' do
+        it 'rejects downloads and uploads with 404 Not Found' do
+          download_or_upload(path, user: user.username, password: user.password) do |response|
+            expect(response).to have_http_status(:not_found)
           end
         end
       end
     end
 
-    context "when the Wiki for a project exists" do
-      it "responds with the right project" do
-        wiki = ProjectWiki.new(project)
-        project.update_attribute(:visibility_level, Project::PUBLIC)
+    context "when requesting the Wiki" do
+      let(:wiki) { ProjectWiki.new(project) }
+      let(:path) { "/#{wiki.repository.path_with_namespace}.git" }
 
-        download("/#{wiki.repository.path_with_namespace}.git") do |response|
-          json_body = ActiveSupport::JSON.decode(response.body)
+      context "when the project is public" do
+        let(:project) { create(:project, :repository, :public, :wiki_enabled) }
 
-          expect(response).to have_http_status(200)
-          expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
-          expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-        end
-      end
+        it_behaves_like 'pushes require Basic HTTP Authentication'
 
-      context 'but the repo is disabled' do
-        let(:project) { create(:project, :repository_disabled, :wiki_enabled) }
-        let(:wiki) { ProjectWiki.new(project) }
-        let(:path) { "/#{wiki.repository.path_with_namespace}.git" }
+        context 'when unauthenticated' do
+          let(:env) { {} }
 
-        before do
-          project.team << [user, :developer]
-        end
+          it_behaves_like 'pulls are allowed'
 
-        it 'allows clones' do
-          download(path, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+          it "responds to pulls with the wiki's repo" do
+            download(path) do |response|
+              json_body = ActiveSupport::JSON.decode(response.body)
+
+              expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+            end
           end
         end
 
-        it 'allows pushes' do
-          upload(path, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+        context 'when authenticated' do
+          let(:env) { { user: user.username, password: user.password } }
+
+          context 'and as a developer on the team' do
+            before do
+              project.team << [user, :developer]
+            end
+
+            context 'but the repo is disabled' do
+              let(:project) { create(:project, :repository, :public, :repository_disabled, :wiki_enabled) }
+
+              it_behaves_like 'pulls are allowed'
+              it_behaves_like 'pushes are allowed'
+            end
+          end
+
+          context 'and not on the team' do
+            it_behaves_like 'pulls are allowed'
+
+            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_wiki_error(:write_to_wiki))
+              end
+            end
+          end
+        end
+      end
+
+      context "when the project is private" do
+        let(:project) { create(:project, :repository, :private, :wiki_enabled) }
+
+        it_behaves_like 'pulls require Basic HTTP Authentication'
+        it_behaves_like 'pushes require Basic HTTP Authentication'
+
+        context 'when authenticated' do
+          context 'and as a developer on the team' do
+            before do
+              project.team << [user, :developer]
+            end
+
+            context 'but the repo is disabled' do
+              let(:project) { create(:project, :repository, :private, :repository_disabled, :wiki_enabled) }
+
+              it 'allows clones' do
+                download(path, user: user.username, password: user.password) do |response|
+                  expect(response).to have_http_status(:ok)
+                end
+              end
+
+              it 'pushes are allowed' do
+                upload(path, user: user.username, password: user.password) do |response|
+                  expect(response).to have_http_status(:ok)
+                end
+              end
+            end
+          end
+
+          context 'and not on the team' do
+            it 'rejects clones with 404 Not Found' do
+              download(path, user: user.username, password: user.password) do |response|
+                expect(response).to have_http_status(:not_found)
+                expect(response.body).to eq(git_access_error(:project_not_found))
+              end
+            end
+
+            it 'rejects pushes with 404 Not Found' do
+              upload(path, user: user.username, password: user.password) do |response|
+                expect(response).to have_http_status(:not_found)
+                expect(response.body).to eq(git_access_error(:project_not_found))
+              end
+            end
           end
         end
       end
@@ -84,49 +225,60 @@ describe 'Git HTTP requests', lib: true do
       let(:path) { "#{project.path_with_namespace}.git" }
 
       context "when the project is public" do
-        before do
-          project.update_attribute(:visibility_level, Project::PUBLIC)
+        let(:project) { create(:project, :repository, :public) }
+
+        it_behaves_like 'pushes require Basic HTTP Authentication'
+
+        context 'when not authenticated' do
+          let(:env) { {} }
+
+          it_behaves_like 'pulls are allowed'
         end
 
-        it "downloads get status 200" do
-          download(path, {}) do |response|
-            expect(response).to have_http_status(200)
-            expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-          end
-        end
-
-        it "uploads get status 401" do
-          upload(path, {}) do |response|
-            expect(response).to have_http_status(401)
-          end
-        end
-
-        context "with correct credentials" do
+        context "when authenticated" do
           let(:env) { { user: user.username, password: user.password } }
 
-          it "uploads get status 403" do
-            upload(path, env) do |response|
-              expect(response).to have_http_status(403)
+          context 'as a developer on the team' do
+            before do
+              project.team << [user, :developer]
             end
-          end
 
-          context 'but git-receive-pack is disabled' do
-            it "responds with status 404" do
-              allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
+            it_behaves_like 'pulls are allowed'
+            it_behaves_like 'pushes are allowed'
 
-              upload(path, env) do |response|
-                expect(response).to have_http_status(403)
+            context 'but git-receive-pack over HTTP is disabled in config' do
+              before do
+                allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
+              end
+
+              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))
+                end
+              end
+            end
+
+            context 'but git-upload-pack over HTTP is disabled in config' do
+              it "rejects pushes with 403 Forbidden" do
+                allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
+
+                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))
+                end
               end
             end
           end
-        end
 
-        context 'but git-upload-pack is disabled' do
-          it "responds with status 404" do
-            allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
+          context 'and not a member of the team' do
+            it_behaves_like 'pulls are allowed'
 
-            download(path, {}) do |response|
-              expect(response).to have_http_status(404)
+            it 'rejects pushes with 403 Forbidden' do
+              upload(path, env) do |response|
+                expect(response).to have_http_status(:forbidden)
+                expect(response.body).to eq(change_access_error(:push_code))
+              end
             end
           end
         end
@@ -141,66 +293,41 @@ describe 'Git HTTP requests', lib: true do
 
         context 'when the repo is public' do
           context 'but the repo is disabled' do
-            it 'does not allow to clone the repo' do
-              project = create(:project, :public, :repository_disabled)
+            let(:project) { create(:project, :public, :repository, :repository_disabled) }
+            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:env) { {} }
 
-              download("#{project.path_with_namespace}.git", {}) do |response|
-                expect(response).to have_http_status(:unauthorized)
-              end
-            end
+            it_behaves_like 'pulls require Basic HTTP Authentication'
+            it_behaves_like 'pushes require Basic HTTP Authentication'
           end
 
           context 'but the repo is enabled' do
-            it 'allows to clone the repo' do
-              project = create(:project, :public, :repository_enabled)
+            let(:project) { create(:project, :public, :repository, :repository_enabled) }
+            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:env) { {} }
 
-              download("#{project.path_with_namespace}.git", {}) do |response|
-                expect(response).to have_http_status(:ok)
-              end
-            end
+            it_behaves_like 'pulls are allowed'
           end
 
           context 'but only project members are allowed' do
-            it 'does not allow to clone the repo' do
-              project = create(:project, :public, :repository_private)
+            let(:project) { create(:project, :public, :repository, :repository_private) }
 
-              download("#{project.path_with_namespace}.git", {}) do |response|
-                expect(response).to have_http_status(:unauthorized)
-              end
-            end
+            it_behaves_like 'pulls require Basic HTTP Authentication'
+            it_behaves_like 'pushes require Basic HTTP Authentication'
           end
         end
       end
 
       context "when the project is private" do
-        before do
-          project.update_attribute(:visibility_level, Project::PRIVATE)
-        end
+        let(:project) { create(:project, :repository, :private) }
 
-        context "when no authentication is provided" do
-          it "responds with status 401 to downloads" do
-            download(path, {}) do |response|
-              expect(response).to have_http_status(401)
-            end
-          end
-
-          it "responds with status 401 to uploads" do
-            upload(path, {}) do |response|
-              expect(response).to have_http_status(401)
-            end
-          end
-        end
+        it_behaves_like 'pulls require Basic HTTP Authentication'
+        it_behaves_like 'pushes require Basic HTTP Authentication'
 
         context "when username and password are provided" do
           let(:env) { { user: user.username, password: 'nope' } }
 
           context "when authentication fails" do
-            it "responds with status 401" do
-              download(path, env) do |response|
-                expect(response).to have_http_status(401)
-              end
-            end
-
             context "when the user is IP banned" do
               it "responds with status 401" do
                 expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
@@ -208,7 +335,7 @@ describe 'Git HTTP requests', lib: true do
 
                 clone_get(path, env)
 
-                expect(response).to have_http_status(401)
+                expect(response).to have_http_status(:unauthorized)
               end
             end
           end
@@ -222,37 +349,39 @@ describe 'Git HTTP requests', lib: true do
               end
 
               context "when the user is blocked" do
-                it "responds with status 401" do
+                it "rejects pulls with 401 Unauthorized" do
                   user.block
                   project.team << [user, :master]
 
                   download(path, env) do |response|
-                    expect(response).to have_http_status(401)
+                    expect(response).to have_http_status(:unauthorized)
                   end
                 end
 
-                it "responds with status 401 for unknown projects (no project existence information leak)" do
+                it "rejects pulls with 401 Unauthorized for unknown projects (no project existence information leak)" do
                   user.block
 
                   download('doesnt/exist.git', env) do |response|
-                    expect(response).to have_http_status(401)
+                    expect(response).to have_http_status(:unauthorized)
                   end
                 end
               end
 
               context "when the user isn't blocked" do
-                it "downloads get status 200" do
-                  expect(Rack::Attack::Allow2Ban).to receive(:reset)
+                it "resets the IP in Rack Attack on download" do
+                  expect(Rack::Attack::Allow2Ban).to receive(:reset).twice
 
-                  clone_get(path, env)
-
-                  expect(response).to have_http_status(200)
-                  expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+                  download(path, env) do
+                    expect(response).to have_http_status(:ok)
+                    expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+                  end
                 end
 
-                it "uploads get status 200" do
-                  upload(path, env) do |response|
-                    expect(response).to have_http_status(200)
+                it "resets the IP in Rack Attack on upload" do
+                  expect(Rack::Attack::Allow2Ban).to receive(:reset).twice
+
+                  upload(path, env) do
+                    expect(response).to have_http_status(:ok)
                     expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
                   end
                 end
@@ -272,56 +401,43 @@ describe 'Git HTTP requests', lib: true do
                   @token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api")
                 end
 
-                it "downloads get status 200" do
-                  clone_get "#{project.path_with_namespace}.git", user: 'oauth2', password: @token.token
+                let(:path) { "#{project.path_with_namespace}.git" }
+                let(:env) { { user: 'oauth2', password: @token.token } }
 
-                  expect(response).to have_http_status(200)
-                  expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-                end
-
-                it "uploads get status 200" do
-                  push_get "#{project.path_with_namespace}.git", user: 'oauth2', password: @token.token
-
-                  expect(response).to have_http_status(200)
-                end
+                it_behaves_like 'pulls are allowed'
+                it_behaves_like 'pushes are allowed'
               end
 
               context 'when user has 2FA enabled' do
                 let(:user) { create(:user, :two_factor) }
                 let(:access_token) { create(:personal_access_token, user: user) }
+                let(:path) { "#{project.path_with_namespace}.git" }
 
                 before do
                   project.team << [user, :master]
                 end
 
                 context 'when username and password are provided' do
-                  it 'rejects the clone attempt' do
-                    download("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
-                      expect(response).to have_http_status(401)
+                  it 'rejects pulls with 2FA error message' do
+                    download(path, user: user.username, password: user.password) do |response|
+                      expect(response).to have_http_status(:unauthorized)
                       expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
                     end
                   end
 
                   it 'rejects the push attempt' do
-                    upload("#{project.path_with_namespace}.git", user: user.username, password: user.password) do |response|
-                      expect(response).to have_http_status(401)
+                    upload(path, user: user.username, password: user.password) do |response|
+                      expect(response).to have_http_status(:unauthorized)
                       expect(response.body).to include('You have 2FA enabled, please use a personal access token for Git over HTTP')
                     end
                   end
                 end
 
                 context 'when username and personal access token are provided' do
-                  it 'allows clones' do
-                    download("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
-                      expect(response).to have_http_status(200)
-                    end
-                  end
+                  let(:env) { { user: user.username, password: access_token.token } }
 
-                  it 'allows pushes' do
-                    upload("#{project.path_with_namespace}.git", user: user.username, password: access_token.token) do |response|
-                      expect(response).to have_http_status(200)
-                    end
-                  end
+                  it_behaves_like 'pulls are allowed'
+                  it_behaves_like 'pushes are allowed'
                 end
               end
 
@@ -357,15 +473,15 @@ describe 'Git HTTP requests', lib: true do
             end
 
             context "when the user doesn't have access to the project" do
-              it "downloads get status 404" do
+              it "pulls get status 404" do
                 download(path, user: user.username, password: user.password) do |response|
-                  expect(response).to have_http_status(404)
+                  expect(response).to have_http_status(:not_found)
                 end
               end
 
               it "uploads get status 404" do
                 upload(path, user: user.username, password: user.password) do |response|
-                  expect(response).to have_http_status(404)
+                  expect(response).to have_http_status(:not_found)
                 end
               end
             end
@@ -378,23 +494,24 @@ describe 'Git HTTP requests', lib: true do
           let(:other_project) { create(:empty_project) }
 
           context 'when build created by system is authenticated' do
-            it "downloads get status 200" do
-              clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
+            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
-              expect(response).to have_http_status(200)
-              expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+            it_behaves_like 'pulls are allowed'
+
+            # TODO Verify this is desired behavior
+            it "rejects pushes with 401 Unauthorized (no project existence information leak)" do
+              push_get(path, env)
+
+              expect(response).to have_http_status(:unauthorized)
             end
 
-            it "uploads get status 401 (no project existence information leak)" do
-              push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
+            # TODO Verify this is desired behavior. Should be 403 Forbidden?
+            it "rejects pulls for other project with 404 Not Found" do
+              clone_get("#{other_project.path_with_namespace}.git", env)
 
-              expect(response).to have_http_status(401)
-            end
-
-            it "downloads from other project get status 404" do
-              clone_get "#{other_project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
-
-              expect(response).to have_http_status(404)
+              expect(response).to have_http_status(:not_found)
+              expect(response.body).to eq('TODO: What should this be?')
             end
           end
 
@@ -412,7 +529,7 @@ describe 'Git HTTP requests', lib: true do
                 clone_get "#{project.path_with_namespace}.git",
                   user: 'gitlab-ci-token', password: build.token
 
-                expect(response).to have_http_status(200)
+                expect(response).to have_http_status(:ok)
                 expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
               end
 
@@ -423,13 +540,13 @@ describe 'Git HTTP requests', lib: true do
                 clone_get "#{project.path_with_namespace}.git",
                   user: 'gitlab-ci-token', password: build.token
 
-                expect(response).to have_http_status(403)
+                expect(response).to have_http_status(:forbidden)
               end
 
               it 'uploads get status 403' do
                 push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
 
-                expect(response).to have_http_status(401)
+                expect(response).to have_http_status(:unauthorized)
               end
             end
 
@@ -441,7 +558,7 @@ describe 'Git HTTP requests', lib: true do
               it 'downloads from other project get status 403' do
                 clone_get "#{other_project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
 
-                expect(response).to have_http_status(403)
+                expect(response).to have_http_status(:forbidden)
               end
             end
 
@@ -453,91 +570,93 @@ describe 'Git HTTP requests', lib: true do
               it 'downloads from other project get status 404' do
                 clone_get "#{other_project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
 
-                expect(response).to have_http_status(404)
+                expect(response).to have_http_status(:not_found)
               end
             end
           end
         end
       end
-    end
 
-    context "when the project path doesn't end in .git" do
-      context "GET info/refs" do
-        let(:path) { "/#{project.path_with_namespace}/info/refs" }
+      context "when the project path doesn't end in .git" do
+        let(:project) { create(:project, :repository, :public, path: 'project.git-project') }
 
-        context "when no params are added" do
-          before { get path }
+        context "GET info/refs" do
+          let(:path) { "/#{project.path_with_namespace}/info/refs" }
 
-          it "redirects to the .git suffix version" do
-            expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs")
+          context "when no params are added" do
+            before { get path }
+
+            it "redirects to the .git suffix version" do
+              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs")
+            end
+          end
+
+          context "when the upload-pack service is requested" do
+            let(:params) { { service: 'git-upload-pack' } }
+            before { get path, params }
+
+            it "redirects to the .git suffix version" do
+              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
+            end
+          end
+
+          context "when the receive-pack service is requested" do
+            let(:params) { { service: 'git-receive-pack' } }
+            before { get path, params }
+
+            it "redirects to the .git suffix version" do
+              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
+            end
+          end
+
+          context "when the params are anything else" do
+            let(:params) { { service: 'git-implode-pack' } }
+            before { get path, params }
+
+            it "redirects to the sign-in page" do
+              expect(response).to redirect_to(new_user_session_path)
+            end
           end
         end
 
-        context "when the upload-pack service is requested" do
-          let(:params) { { service: 'git-upload-pack' } }
-          before { get path, params }
-
-          it "redirects to the .git suffix version" do
-            expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
+        context "POST git-upload-pack" do
+          it "fails to find a route" do
+            expect { clone_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
           end
         end
 
-        context "when the receive-pack service is requested" do
-          let(:params) { { service: 'git-receive-pack' } }
-          before { get path, params }
-
-          it "redirects to the .git suffix version" do
-            expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
-          end
-        end
-
-        context "when the params are anything else" do
-          let(:params) { { service: 'git-implode-pack' } }
-          before { get path, params }
-
-          it "redirects to the sign-in page" do
-            expect(response).to redirect_to(new_user_session_path)
+        context "POST git-receive-pack" do
+          it "failes to find a route" do
+            expect { push_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
           end
         end
       end
 
-      context "POST git-upload-pack" do
-        it "fails to find a route" do
-          expect { clone_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
-        end
-      end
+      context "retrieving an info/refs file" do
+        let(:project) { create(:project, :repository, :public) }
 
-      context "POST git-receive-pack" do
-        it "failes to find a route" do
-          expect { push_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
-        end
-      end
-    end
+        context "when the file exists" do
+          before do
+            # Provide a dummy file in its place
+            allow_any_instance_of(Repository).to receive(:blob_at).and_call_original
+            allow_any_instance_of(Repository).to receive(:blob_at).with('b83d6e391c22777fca1ed3012fce84f633d7fed0', 'info/refs') do
+              Gitlab::Git::Blob.find(project.repository, 'master', 'bar/branch-test.txt')
+            end
 
-    context "retrieving an info/refs file" do
-      before { project.update_attribute(:visibility_level, Project::PUBLIC) }
-
-      context "when the file exists" do
-        before do
-          # Provide a dummy file in its place
-          allow_any_instance_of(Repository).to receive(:blob_at).and_call_original
-          allow_any_instance_of(Repository).to receive(:blob_at).with('b83d6e391c22777fca1ed3012fce84f633d7fed0', 'info/refs') do
-            Gitlab::Git::Blob.find(project.repository, 'master', 'bar/branch-test.txt')
+            get "/#{project.path_with_namespace}/blob/master/info/refs"
           end
 
-          get "/#{project.path_with_namespace}/blob/master/info/refs"
+          it "returns the file" do
+            expect(response).to have_http_status(:ok)
+          end
         end
 
-        it "returns the file" do
-          expect(response).to have_http_status(200)
-        end
-      end
+        context "when the file does not exist" do
+          before { get "/#{project.path_with_namespace}/blob/master/info/refs" }
 
-      context "when the file does not exist" do
-        before { get "/#{project.path_with_namespace}/blob/master/info/refs" }
-
-        it "returns not found" do
-          expect(response).to have_http_status(404)
+          it "returns not found" do
+            expect(response).to have_http_status(:not_found)
+          end
         end
       end
     end
@@ -546,6 +665,7 @@ describe 'Git HTTP requests', lib: true do
   describe "User with LDAP identity" do
     let(:user) { create(:omniauth_user, extern_uid: dn) }
     let(:dn) { 'uid=john,ou=people,dc=example,dc=com' }
+    let(:path) { 'doesnt/exist.git' }
 
     before do
       allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
@@ -553,44 +673,36 @@ describe 'Git HTTP requests', lib: true do
       allow(Gitlab::LDAP::Authentication).to receive(:login).with(user.username, user.password).and_return(user)
     end
 
-    context "when authentication fails" do
-      context "when no authentication is provided" do
-        it "responds with status 401" do
-          download('doesnt/exist.git') do |response|
-            expect(response).to have_http_status(401)
-          end
-        end
-      end
-
-      context "when username and invalid password are provided" do
-        it "responds with status 401" do
-          download('doesnt/exist.git', user: user.username, password: "nope") do |response|
-            expect(response).to have_http_status(401)
-          end
-        end
-      end
-    end
+    it_behaves_like 'pulls require Basic HTTP Authentication'
+    it_behaves_like 'pushes require Basic HTTP Authentication'
 
     context "when authentication succeeds" do
       context "when the project doesn't exist" do
-        it "responds with status 404" do
-          download('/doesnt/exist.git', user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(404)
+        it "responds with status 404 Not Found" do
+          download(path, user: user.username, password: user.password) do |response|
+            expect(response).to have_http_status(:not_found)
           end
         end
       end
 
       context "when the project exists" do
-        let(:project) { create(:project, path: 'project.git-project') }
+        let(:project) { create(:project, :repository) }
+        let(:path) { "#{project.full_path}.git" }
+        let(:env) { { user: user.username, password: user.password } }
 
-        before do
-          project.team << [user, :master]
-        end
-
-        it "responds with status 200" do
-          clone_get(path, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+        context 'and the user is on the team' do
+          before do
+            project.team << [user, :master]
           end
+
+          it "responds with status 200" do
+            clone_get(path, env) do |response|
+              expect(response).to have_http_status(200)
+            end
+          end
+
+          it_behaves_like 'pulls are allowed'
+          it_behaves_like 'pushes are allowed'
         end
       end
     end
diff --git a/spec/support/git_http_helpers.rb b/spec/support/git_http_helpers.rb
index 46b686fce94..d3d51560a9d 100644
--- a/spec/support/git_http_helpers.rb
+++ b/spec/support/git_http_helpers.rb
@@ -35,9 +35,14 @@ module GitHttpHelpers
     yield response
   end
 
+  def download_or_upload(*args, &block)
+    download(*args, &block)
+    upload(*args, &block)
+  end
+
   def auth_env(user, password, spnego_request_token)
     env = workhorse_internal_api_request_header
-    if user && password
+    if user
       env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(user, password)
     elsif spnego_request_token
       env['HTTP_AUTHORIZATION'] = "Negotiate #{::Base64.strict_encode64('opaque_request_token')}"
@@ -45,4 +50,16 @@ module GitHttpHelpers
 
     env
   end
+
+  def git_access_error(error_key)
+    Gitlab::GitAccess::ERROR_MESSAGES[error_key]
+  end
+
+  def git_access_wiki_error(error_key)
+    Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+  end
+
+  def change_access_error(error_key)
+    Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+  end
 end

From 2d6cafa781ae24586fcd5307ae01daf3f407aa25 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 15 May 2017 16:19:36 -0700
Subject: [PATCH 02/16] Render GitAccess message if authorized

---
 app/controllers/projects/git_http_controller.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 9e4edcae101..a36dc362f4e 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -68,17 +68,13 @@ class Projects::GitHttpController < Projects::GitHttpClientController
 
   def render_denied
     if user && can?(user, :read_project, project)
-      render plain: access_denied_message, status: :forbidden
+      render plain: access_check.message, status: :forbidden
     else
       # Do not leak information about project existence
       render_not_found
     end
   end
 
-  def access_denied_message
-    'Access denied'
-  end
-
   def upload_pack_allowed?
     return false unless Gitlab.config.gitlab_shell.upload_pack
 

From a738a446f4ade6204c10f016e355da354dbfc01f Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 16 May 2017 09:02:52 -0700
Subject: [PATCH 03/16] Check disabled commands in GitAccess instead

---
 .../projects/git_http_controller.rb           |  4 --
 lib/gitlab/git_access.rb                      | 27 +++++++++++-
 spec/lib/gitlab/git_access_spec.rb            | 43 ++++++++++++++++++-
 3 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index a36dc362f4e..e7b498599f2 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -76,8 +76,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def upload_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.upload_pack
-
     access_check.allowed? || ci?
   end
 
@@ -96,8 +94,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def receive_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.receive_pack
-
     access_check.allowed?
   end
 
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 591f68cd415..1d052ac9b33 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -12,7 +12,9 @@ module Gitlab
       no_repo: 'A repository for this project does not exist yet.',
       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."
+      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.'
     }.freeze
 
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze
@@ -33,6 +35,7 @@ module Gitlab
       check_protocol!
       check_active_user!
       check_project_accessibility!
+      check_command_disabled!(cmd)
       check_command_existence!(cmd)
       check_repository_existence!
 
@@ -86,6 +89,16 @@ module Gitlab
       end
     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
+      end
+    end
+
     def check_command_existence!(cmd)
       unless ALL_COMMANDS.include?(cmd)
         raise UnauthorizedError, ERROR_MESSAGES[:command_not_allowed]
@@ -179,6 +192,18 @@ module Gitlab
       end || Guest.can?(:read_project, project)
     end
 
+    def http?
+      protocol == 'http'
+    end
+
+    def upload_pack?(command)
+      command == 'git-upload-pack'
+    end
+
+    def receive_pack?(command)
+      command == 'git-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 25769977f24..a86afe57873 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -1,10 +1,11 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccess, lib: true do
-  let(:access) { Gitlab::GitAccess.new(actor, project, 'ssh', authentication_abilities: authentication_abilities) }
+  let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) }
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:actor) { user }
+  let(:protocol) { 'ssh' }
   let(:authentication_abilities) do
     [
       :read_project,
@@ -50,6 +51,46 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
+  describe '#check with commands disabled' do
+    before { project.team << [user, :master] }
+
+    context 'over http' do
+      let(:protocol) { 'http' }
+
+      context 'when the git-upload-pack command is disabled in config' do
+        before do
+          allow(Gitlab.config.gitlab_shell).to receive(:upload_pack).and_return(false)
+        end
+
+        context 'when calling git-upload-pack' do
+          subject { access.check('git-upload-pack', '_any') }
+          it { expect(subject.allowed?).to be_falsey }
+          it { expect(subject.message).to eq('The command "git-upload-pack" is not allowed.') }
+        end
+
+        context 'when calling git-receive-pack' do
+          it { expect(access.check('git-receive-pack', '_any').allowed?).to be_truthy }
+        end
+      end
+
+      context 'when the git-receive-pack command is disabled in config' do
+        before do
+          allow(Gitlab.config.gitlab_shell).to receive(:receive_pack).and_return(false)
+        end
+
+        context 'when calling git-receive-pack' do
+          subject { access.check('git-receive-pack', '_any') }
+          it { expect(subject.allowed?).to be_falsey }
+          it { expect(subject.message).to eq('The command "git-receive-pack" is not allowed.') }
+        end
+
+        context 'when calling git-upload-pack' do
+          it { expect(access.check('git-upload-pack', '_any').allowed?).to be_truthy }
+        end
+      end
+    end
+  end
+
   describe '#check_download_access!' do
     subject { access.check('git-upload-pack', '_any') }
 

From b387429458f77a3608e077dfe2d50b0a313f8832 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 16 May 2017 09:08:23 -0700
Subject: [PATCH 04/16] Refactor

---
 .../projects/git_http_client_controller.rb           |  4 ----
 spec/lib/gitlab/git_access_spec.rb                   | 12 ++++++------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 9a1bf037a95..44b0853e3e9 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -152,8 +152,4 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def has_authentication_ability?(capability)
     (authentication_abilities || []).include?(capability)
   end
-
-  def authentication_project
-    authentication_result.project
-  end
 end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index a86afe57873..1033fef9684 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -23,30 +23,30 @@ describe Gitlab::GitAccess, lib: true do
     context 'ssh disabled' do
       before do
         disable_protocol('ssh')
-        @acc = Gitlab::GitAccess.new(actor, project, 'ssh', authentication_abilities: authentication_abilities)
       end
 
       it 'blocks ssh git push' do
-        expect(@acc.check('git-receive-pack', '_any').allowed?).to be_falsey
+        expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey
       end
 
       it 'blocks ssh git pull' do
-        expect(@acc.check('git-upload-pack', '_any').allowed?).to be_falsey
+        expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey
       end
     end
 
     context 'http disabled' do
+      let(:protocol) { 'http' }
+
       before do
         disable_protocol('http')
-        @acc = Gitlab::GitAccess.new(actor, project, 'http', authentication_abilities: authentication_abilities)
       end
 
       it 'blocks http push' do
-        expect(@acc.check('git-receive-pack', '_any').allowed?).to be_falsey
+        expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey
       end
 
       it 'blocks http git pull' do
-        expect(@acc.check('git-upload-pack', '_any').allowed?).to be_falsey
+        expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey
       end
     end
   end

From bad08fbea2a32655a6d87f2140840c317cea6c80 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 16 May 2017 12:58:46 -0700
Subject: [PATCH 05/16] Move CI access logic into GitAccess

---
 app/controllers/concerns/lfs_request.rb       |   4 +
 .../projects/git_http_client_controller.rb    |  20 +--
 .../projects/git_http_controller.rb           |  16 ++-
 lib/gitlab/git_access.rb                      |  20 ++-
 spec/lib/gitlab/git_access_spec.rb            | 128 +++++++++++++++++-
 spec/requests/git_http_spec.rb                |  58 ++++----
 spec/support/git_http_helpers.rb              |   9 +-
 7 files changed, 198 insertions(+), 57 deletions(-)

diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb
index ae91e02488a..2b6afaa6233 100644
--- a/app/controllers/concerns/lfs_request.rb
+++ b/app/controllers/concerns/lfs_request.rb
@@ -106,4 +106,8 @@ module LfsRequest
   def objects
     @objects ||= (params[:objects] || []).to_a
   end
+
+  def has_authentication_ability?(capability)
+    (authentication_abilities || []).include?(capability)
+  end
 end
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 44b0853e3e9..7f3205a8001 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -128,28 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @authentication_result = Gitlab::Auth.find_for_git_client(
       login, password, project: project, ip: request.ip)
 
-    return false unless @authentication_result.success?
-
-    if download_request?
-      authentication_has_download_access?
-    else
-      authentication_has_upload_access?
-    end
+    @authentication_result.success?
   end
 
   def ci?
     authentication_result.ci?(project)
   end
-
-  def authentication_has_download_access?
-    has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
-  end
-
-  def authentication_has_upload_access?
-    has_authentication_ability?(:push_code)
-  end
-
-  def has_authentication_ability?(capability)
-    (authentication_abilities || []).include?(capability)
-  end
 end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index e7b498599f2..2c2766cf623 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -67,20 +67,24 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def render_denied
-    if user && can?(user, :read_project, project)
-      render plain: access_check.message, status: :forbidden
+    if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]
+      render plain: access_check.message, status: :not_found
     else
-      # Do not leak information about project existence
-      render_not_found
+      render plain: access_check.message, status: :forbidden
     end
   end
 
   def upload_pack_allowed?
-    access_check.allowed? || ci?
+    access_check.allowed?
   end
 
   def access
-    @access ||= access_klass.new(user, project, 'http', authentication_abilities: authentication_abilities)
+    @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
+  end
+
+  def access_actor
+    return user if user
+    return :ci if ci?
   end
 
   def access_check
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 1d052ac9b33..1ffac5385c2 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -63,6 +63,11 @@ module Gitlab
       authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
     end
 
+    # Allow generic CI (build without a user) for backwards compatibility
+    def ci_can_download_code?
+      authentication_abilities.include?(:build_download_code) && ci?
+    end
+
     def protocol_allowed?
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
@@ -115,6 +120,7 @@ module Gitlab
       return if deploy_key?
 
       passed = user_can_download_code? ||
+        ci_can_download_code? ||
         build_can_download_code? ||
         guest_can_download_code?
 
@@ -184,11 +190,17 @@ module Gitlab
       actor.is_a?(DeployKey)
     end
 
+    def ci?
+      actor == :ci
+    end
+
     def can_read_project?
-      if deploy_key
+      if deploy_key?
         deploy_key.has_access_to?(project)
       elsif user
         user.can?(:read_project, project)
+      elsif ci?
+        true # allow CI (build without a user) for backwards compatibility
       end || Guest.can?(:read_project, project)
     end
 
@@ -213,10 +225,12 @@ module Gitlab
         case actor
         when User
           actor
-        when DeployKey
-          nil
         when Key
           actor.user
+        when DeployKey
+          nil
+        when :ci
+          nil
         end
     end
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 1033fef9684..0efe15856fc 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccess, lib: true do
+  let(:pull_access_check) { access.check('git-upload-pack', '_any') }
+  let(:push_access_check) { access.check('git-receive-pack', '_any') }
   let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) }
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
@@ -51,7 +53,123 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe '#check with commands disabled' do
+  describe '#check_project_accessibility!' do
+    context 'when the project exists' do
+      context 'when actor exists' do
+        context 'when actor is a DeployKey' do
+          let(:deploy_key) { create(:deploy_key, user: user, can_push: true) }
+          let(:actor) { deploy_key }
+
+          context 'when the DeployKey has access to the project' do
+            before { deploy_key.projects << project }
+
+            it 'allows pull access' do
+              expect(pull_access_check.allowed?).to be_truthy
+            end
+
+            it 'allows push access' do
+              expect(push_access_check.allowed?).to be_truthy
+            end
+          end
+
+          context 'when the Deploykey does not have access to the project' do
+            it 'blocks pulls with "not found"' do
+              expect(pull_access_check.allowed?).to be_falsey
+              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+
+            it 'blocks pushes with "not found"' do
+              expect(push_access_check.allowed?).to be_falsey
+              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+          end
+        end
+
+        context 'when actor is a User' do
+          context 'when the User can read the project' do
+            before { project.team << [user, :master] }
+
+            it 'allows pull access' do
+              expect(pull_access_check.allowed?).to be_truthy
+            end
+
+            it 'allows push access' do
+              expect(push_access_check.allowed?).to be_truthy
+            end
+          end
+
+          context 'when the User cannot read the project' do
+            it 'blocks pulls with "not found"' do
+              expect(pull_access_check.allowed?).to be_falsey
+              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+
+            it 'blocks pushes with "not found"' do
+              expect(push_access_check.allowed?).to be_falsey
+              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+          end
+        end
+
+        # For backwards compatibility
+        context 'when actor is :ci' do
+          let(:actor) { :ci }
+          let(:authentication_abilities) { build_authentication_abilities }
+
+          it 'allows pull access' do
+            expect(pull_access_check.allowed?).to be_truthy
+          end
+
+          it 'does not block pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+          end
+        end
+      end
+
+      context 'when actor is nil' do
+        let(:actor) { nil }
+
+        context 'when guests can read the project' do
+          let(:project) { create(:project, :repository, :public) }
+
+          it 'allows pull access' do
+            expect(pull_access_check.allowed?).to be_truthy
+          end
+
+          it 'does not block pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+          end
+        end
+
+        context 'when guests cannot read the project' do
+          it 'blocks pulls with "not found"' do
+            expect(pull_access_check.allowed?).to be_falsey
+            expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+          end
+
+          it 'blocks pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+          end
+        end
+      end
+    end
+
+    context 'when the project is nil' do
+      let(:project) { nil }
+
+      it 'blocks any command with "not found"' do
+        expect(pull_access_check.allowed?).to be_falsey
+        expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+        expect(push_access_check.allowed?).to be_falsey
+        expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+      end
+    end
+  end
+
+  describe '#check_command_disabled!' do
     before { project.team << [user, :master] }
 
     context 'over http' do
@@ -219,6 +337,14 @@ describe Gitlab::GitAccess, lib: true do
           end
         end
       end
+
+      describe 'generic CI (build without a user)' do
+        let(:actor) { :ci }
+
+        context 'pull code' do
+          it { expect(subject).to be_allowed }
+        end
+      end
     end
   end
 
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 080e2f12cd7..ab7c56fcdf0 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -489,29 +489,41 @@ describe 'Git HTTP requests', lib: true do
         end
 
         context "when a gitlab ci token is provided" do
+          let(:project) { create(:project, :repository) }
           let(:build) { create(:ci_build, :running) }
-          let(:project) { build.project }
           let(:other_project) { create(:empty_project) }
 
+          before do
+            build.update!(project: project) # can't associate it on factory create
+          end
+
           context 'when build created by system is authenticated' do
             let(:path) { "#{project.path_with_namespace}.git" }
             let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
             it_behaves_like 'pulls are allowed'
 
-            # TODO Verify this is desired behavior
-            it "rejects pushes with 401 Unauthorized (no project existence information leak)" do
+            # A non-401 here is not an information leak since the system is
+            # "authenticated" as CI using the correct token. It does not have
+            # push access, so pushes should be rejected as forbidden, and giving
+            # a reason is fine.
+            #
+            # We know for sure it is not an information leak since pulls using
+            # the build token must be allowed.
+            it "rejects pushes with 403 Forbidden" do
               push_get(path, env)
 
-              expect(response).to have_http_status(:unauthorized)
+              expect(response).to have_http_status(:forbidden)
+              expect(response.body).to eq(git_access_error(:upload))
             end
 
-            # TODO Verify this is desired behavior. Should be 403 Forbidden?
+            # We are "authenticated" as CI using a valid token here. But we are
+            # not authorized to see any other project, so return "not found".
             it "rejects pulls for other project with 404 Not Found" do
               clone_get("#{other_project.path_with_namespace}.git", env)
 
               expect(response).to have_http_status(:not_found)
-              expect(response.body).to eq('TODO: What should this be?')
+              expect(response.body).to eq(git_access_error(:project_not_found))
             end
           end
 
@@ -522,31 +534,27 @@ describe 'Git HTTP requests', lib: true do
             end
 
             shared_examples 'can download code only' do
-              it 'downloads get status 200' do
-                allow_any_instance_of(Repository).
-                  to receive(:exists?).and_return(true)
+              let(:path) { "#{project.path_with_namespace}.git" }
+              let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
-                clone_get "#{project.path_with_namespace}.git",
-                  user: 'gitlab-ci-token', password: build.token
+              it_behaves_like 'pulls are allowed'
 
-                expect(response).to have_http_status(:ok)
-                expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+              context 'when the repo does not exist' do
+                let(:project) { create(:empty_project) }
+                
+                it 'rejects pulls with 403 Forbidden' do
+                  clone_get path, env
+
+                  expect(response).to have_http_status(:forbidden)
+                  expect(response.body).to eq(git_access_error(:no_repo))
+                end
               end
 
-              it 'downloads from non-existing repository and gets 403' do
-                allow_any_instance_of(Repository).
-                  to receive(:exists?).and_return(false)
-
-                clone_get "#{project.path_with_namespace}.git",
-                  user: 'gitlab-ci-token', password: build.token
+              it 'rejects pushes with 403 Forbidden' do
+                push_get path, env
 
                 expect(response).to have_http_status(:forbidden)
-              end
-
-              it 'uploads get status 403' do
-                push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
-
-                expect(response).to have_http_status(:unauthorized)
+                expect(response.body).to eq(git_access_error(:upload))
               end
             end
 
diff --git a/spec/support/git_http_helpers.rb b/spec/support/git_http_helpers.rb
index d3d51560a9d..b8289e6c5f1 100644
--- a/spec/support/git_http_helpers.rb
+++ b/spec/support/git_http_helpers.rb
@@ -52,14 +52,17 @@ module GitHttpHelpers
   end
 
   def git_access_error(error_key)
-    Gitlab::GitAccess::ERROR_MESSAGES[error_key]
+    message = Gitlab::GitAccess::ERROR_MESSAGES[error_key]
+    message || raise("GitAccess error message key '#{error_key}' not found")
   end
 
   def git_access_wiki_error(error_key)
-    Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+    message = Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+    message || raise("GitAccessWiki error message key '#{error_key}' not found")
   end
 
   def change_access_error(error_key)
-    Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+    message = Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+    message || raise("ChangeAccess error message key '#{error_key}' not found")
   end
 end

From 9d78f83571e7dbfbab889102b497ada7c02f409d Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Wed, 17 May 2017 11:00:36 -0700
Subject: [PATCH 06/16] Specify new Git-LFS-over-HTTP behavior
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Changes to `GitHttpClientController`’s authentication logic caused this behavior change.

The old 401 Unauthorized statuses didn’t cause any harm, but they weren’t quite as accurate as the new behavior.
---
 spec/requests/lfs_http_spec.rb | 38 ++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 0c9b4121adf..697b150ab34 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -759,8 +759,8 @@ describe 'Git LFS API and storage' do
             context 'tries to push to own project' do
               let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-              it 'responds with 401' do
-                expect(response).to have_http_status(401)
+              it 'responds with 403 (not 404 because project is public)' do
+                expect(response).to have_http_status(403)
               end
             end
 
@@ -769,8 +769,9 @@ describe 'Git LFS API and storage' do
               let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
               let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-              it 'responds with 401' do
-                expect(response).to have_http_status(401)
+              # I'm not sure what this tests that is different from the previous test
+              it 'responds with 403 (not 404 because project is public)' do
+                expect(response).to have_http_status(403)
               end
             end
           end
@@ -778,8 +779,8 @@ describe 'Git LFS API and storage' do
           context 'does not have user' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
-            it 'responds with 401' do
-              expect(response).to have_http_status(401)
+            it 'responds with 403 (not 404 because project is public)' do
+              expect(response).to have_http_status(403)
             end
           end
         end
@@ -979,8 +980,8 @@ describe 'Git LFS API and storage' do
               put_authorize
             end
 
-            it 'responds with 401' do
-              expect(response).to have_http_status(401)
+            it 'responds with 403 (not 404 because the build user can read the project)' do
+              expect(response).to have_http_status(403)
             end
           end
 
@@ -993,8 +994,8 @@ describe 'Git LFS API and storage' do
               put_authorize
             end
 
-            it 'responds with 401' do
-              expect(response).to have_http_status(401)
+            it 'responds with 404 (do not leak non-public project existence)' do
+              expect(response).to have_http_status(404)
             end
           end
         end
@@ -1006,8 +1007,8 @@ describe 'Git LFS API and storage' do
             put_authorize
           end
 
-          it 'responds with 401' do
-            expect(response).to have_http_status(401)
+          it 'responds with 404 (do not leak non-public project existence)' do
+            expect(response).to have_http_status(404)
           end
         end
       end
@@ -1079,8 +1080,8 @@ describe 'Git LFS API and storage' do
           context 'tries to push to own project' do
             let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-            it 'responds with 401' do
-              expect(response).to have_http_status(401)
+            it 'responds with 403 (not 404 because project is public)' do
+              expect(response).to have_http_status(403)
             end
           end
 
@@ -1089,8 +1090,9 @@ describe 'Git LFS API and storage' do
             let(:pipeline) { create(:ci_empty_pipeline, project: other_project) }
             let(:build) { create(:ci_build, :running, pipeline: pipeline, user: user) }
 
-            it 'responds with 401' do
-              expect(response).to have_http_status(401)
+            # I'm not sure what this tests that is different from the previous test
+            it 'responds with 403 (not 404 because project is public)' do
+              expect(response).to have_http_status(403)
             end
           end
         end
@@ -1098,8 +1100,8 @@ describe 'Git LFS API and storage' do
         context 'does not have user' do
           let(:build) { create(:ci_build, :running, pipeline: pipeline) }
 
-          it 'responds with 401' do
-            expect(response).to have_http_status(401)
+          it 'responds with 403 (not 404 because project is public)' do
+            expect(response).to have_http_status(403)
           end
         end
       end

From 957edb13fdb21e21efbc68fc342209f4b53a66e4 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Wed, 17 May 2017 15:47:59 -0700
Subject: [PATCH 07/16] Refactor to let `GitAccess` check protocol config

This already works due to previous refactoring.
---
 app/controllers/projects/git_http_controller.rb | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 2c2766cf623..073c76933c1 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -10,8 +10,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
       render_ok
     elsif receive_pack? && receive_pack_allowed?
       render_ok
-    elsif http_blocked?
-      render_http_not_allowed
     else
       render_denied
     end
@@ -62,10 +60,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name)
   end
 
-  def render_http_not_allowed
-    render plain: access_check.message, status: :forbidden
-  end
-
   def render_denied
     if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]
       render plain: access_check.message, status: :not_found
@@ -93,10 +87,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     @access_check ||= access.check(git_command, '_any')
   end
 
-  def http_blocked?
-    !access.protocol_allowed?
-  end
-
   def receive_pack_allowed?
     access_check.allowed?
   end

From 23d37382dabe3f7c7f2e11df2731de8e939e0cab Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Fri, 19 May 2017 12:58:45 -0700
Subject: [PATCH 08/16] Refactor to let GitAccess errors bubble up

No external behavior change.

This allows `GitHttpController` to set the HTTP status based on the type of error. Alternatively, we could have added an attribute to GitAccessStatus, but this pattern seemed appropriate.
---
 .../projects/git_http_controller.rb           |  49 ++---
 lib/api/internal.rb                           |  42 ++--
 lib/gitlab/checks/change_access.rb            |  32 ++-
 lib/gitlab/git_access.rb                      |   9 +-
 lib/gitlab/git_access_status.rb               |   4 -
 lib/gitlab/git_access_wiki.rb                 |   2 +-
 spec/lib/gitlab/checks/change_access_spec.rb  |  57 +++---
 spec/lib/gitlab/git_access_spec.rb            | 187 ++++++++++--------
 spec/lib/gitlab/git_access_wiki_spec.rb       |   7 +-
 9 files changed, 184 insertions(+), 205 deletions(-)

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 073c76933c1..b6b62da7b60 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,36 +1,27 @@
 class Projects::GitHttpController < Projects::GitHttpClientController
   include WorkhorseRequest
 
+  before_action :access_check
+
+  rescue_from Gitlab::GitAccess::UnauthorizedError, with: :render_403
+  rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404
+
   # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
   def info_refs
-    if upload_pack? && upload_pack_allowed?
-      log_user_activity
+    log_user_activity if upload_pack?
 
-      render_ok
-    elsif receive_pack? && receive_pack_allowed?
-      render_ok
-    else
-      render_denied
-    end
+    render_ok
   end
 
   # POST /foo/bar.git/git-upload-pack (git pull)
   def git_upload_pack
-    if upload_pack? && upload_pack_allowed?
-      render_ok
-    else
-      render_denied
-    end
+    render_ok
   end
 
   # POST /foo/bar.git/git-receive-pack" (git push)
   def git_receive_pack
-    if receive_pack? && receive_pack_allowed?
-      render_ok
-    else
-      render_denied
-    end
+    render_ok
   end
 
   private
@@ -43,10 +34,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     git_command == 'git-upload-pack'
   end
 
-  def receive_pack?
-    git_command == 'git-receive-pack'
-  end
-
   def git_command
     if action_name == 'info_refs'
       params[:service]
@@ -60,16 +47,12 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name)
   end
 
-  def render_denied
-    if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]
-      render plain: access_check.message, status: :not_found
-    else
-      render plain: access_check.message, status: :forbidden
-    end
+  def render_403(exception)
+    render plain: exception.message, status: :forbidden
   end
 
-  def upload_pack_allowed?
-    access_check.allowed?
+  def render_404(exception)
+    render plain: exception.message, status: :not_found
   end
 
   def access
@@ -84,11 +67,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   def access_check
     # Use the magic string '_any' to indicate we do not know what the
     # changes are. This is also what gitlab-shell does.
-    @access_check ||= access.check(git_command, '_any')
-  end
-
-  def receive_pack_allowed?
-    access_check.allowed?
+    access.check(git_command, '_any')
   end
 
   def access_klass
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 9ebd4841296..3d60d1da114 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -32,30 +32,32 @@ module API
 
         actor.update_last_used_at if actor.is_a?(Key)
 
-        access_checker = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
-        access_status = access_checker
+        access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
+        access_checker = access_checker_klass
           .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
-          .check(params[:action], params[:changes])
 
-        response = { status: access_status.status, message: access_status.message }
-
-        if access_status.status
-          log_user_activity(actor)
-
-          # Project id to pass between components that don't share/don't have
-          # access to the same filesystem mounts
-          response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?)
-
-          # Return the repository full path so that gitlab-shell has it when
-          # handling ssh commands
-          response[:repository_path] =
-            if wiki?
-              project.wiki.repository.path_to_repo
-            else
-              project.repository.path_to_repo
-            end
+        begin
+          access_status = access_checker.check(params[:action], params[:changes])
+          response = { status: access_status.status, message: access_status.message }
+        rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
+          return { status: false, message: e.message }
         end
 
+        log_user_activity(actor)
+
+        # Project id to pass between components that don't share/don't have
+        # access to the same filesystem mounts
+        response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?)
+
+        # Return the repository full path so that gitlab-shell has it when
+        # handling ssh commands
+        response[:repository_path] =
+          if wiki?
+            project.wiki.repository.path_to_repo
+          else
+            project.repository.path_to_repo
+          end
+
         response
       end
 
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 4b6f8abd61d..e9782623be5 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -33,20 +33,18 @@ module Gitlab
       def exec
         return GitAccessStatus.new(true) if skip_authorization
 
-        error = push_checks || branch_checks || tag_checks
+        push_checks
+        branch_checks
+        tag_checks
 
-        if error
-          GitAccessStatus.new(false, error)
-        else
-          GitAccessStatus.new(true)
-        end
+        GitAccessStatus.new(true)
       end
 
       protected
 
       def push_checks
         if user_access.cannot_do_action?(:push_code)
-          ERROR_MESSAGES[:push_code]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
         end
       end
 
@@ -54,7 +52,7 @@ module Gitlab
         return unless @branch_name
 
         if deletion? && @branch_name == project.default_branch
-          return ERROR_MESSAGES[:delete_default_branch]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:delete_default_branch]
         end
 
         protected_branch_checks
@@ -64,7 +62,7 @@ module Gitlab
         return unless ProtectedBranch.protected?(project, @branch_name)
 
         if forced_push?
-          return ERROR_MESSAGES[:force_push_protected_branch]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:force_push_protected_branch]
         end
 
         if deletion?
@@ -76,22 +74,22 @@ module Gitlab
 
       def protected_branch_deletion_checks
         unless user_access.can_delete_branch?(@branch_name)
-          return ERROR_MESSAGES[:non_master_delete_protected_branch]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_master_delete_protected_branch]
         end
 
         unless protocol == 'web'
-          ERROR_MESSAGES[:non_web_delete_protected_branch]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_web_delete_protected_branch]
         end
       end
 
       def protected_branch_push_checks
         if matching_merge_request?
           unless user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
-            ERROR_MESSAGES[:merge_protected_branch]
+            raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:merge_protected_branch]
           end
         else
           unless user_access.can_push_to_branch?(@branch_name)
-            ERROR_MESSAGES[:push_protected_branch]
+            raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_protected_branch]
           end
         end
       end
@@ -100,7 +98,7 @@ module Gitlab
         return unless @tag_name
 
         if tag_exists? && user_access.cannot_do_action?(:admin_project)
-          return ERROR_MESSAGES[:change_existing_tags]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:change_existing_tags]
         end
 
         protected_tag_checks
@@ -109,11 +107,11 @@ module Gitlab
       def protected_tag_checks
         return unless ProtectedTag.protected?(project, @tag_name)
 
-        return ERROR_MESSAGES[:update_protected_tag] if update?
-        return ERROR_MESSAGES[:delete_protected_tag] if deletion?
+        raise(GitAccess::UnauthorizedError, ERROR_MESSAGES[:update_protected_tag]) if update?
+        raise(GitAccess::UnauthorizedError, ERROR_MESSAGES[:delete_protected_tag]) if deletion?
 
         unless user_access.can_create_tag?(@tag_name)
-          return ERROR_MESSAGES[:create_protected_tag]
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:create_protected_tag]
         end
       end
 
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 1ffac5385c2..f43359d7dbd 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -3,6 +3,7 @@
 module Gitlab
   class GitAccess
     UnauthorizedError = Class.new(StandardError)
+    NotFoundError = Class.new(StandardError)
 
     ERROR_MESSAGES = {
       upload: 'You are not allowed to upload code for this project.',
@@ -47,8 +48,6 @@ module Gitlab
       end
 
       build_status_object(true)
-    rescue UnauthorizedError => ex
-      build_status_object(false, ex.message)
     end
 
     def guest_can_download_code?
@@ -90,7 +89,7 @@ module Gitlab
 
     def check_project_accessibility!
       if project.blank? || !can_read_project?
-        raise UnauthorizedError, ERROR_MESSAGES[:project_not_found]
+        raise NotFoundError, ERROR_MESSAGES[:project_not_found]
       end
     end
 
@@ -234,8 +233,8 @@ module Gitlab
         end
     end
 
-    def build_status_object(status, message = '')
-      Gitlab::GitAccessStatus.new(status, message)
+    def build_status_object(status)
+      Gitlab::GitAccessStatus.new(status)
     end
   end
 end
diff --git a/lib/gitlab/git_access_status.rb b/lib/gitlab/git_access_status.rb
index 09bb01be694..94edf80c0f6 100644
--- a/lib/gitlab/git_access_status.rb
+++ b/lib/gitlab/git_access_status.rb
@@ -7,9 +7,5 @@ module Gitlab
       @status = status
       @message = message
     end
-
-    def to_json(opts = nil)
-      { status: @status, message: @message }.to_json(opts)
-    end
   end
 end
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index dccafd2cae8..4c87482430f 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -16,7 +16,7 @@ module Gitlab
       if user_access.can_do_action?(:create_wiki)
         build_status_object(true)
       else
-        build_status_object(false, ERROR_MESSAGES[:write_to_wiki])
+        raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
       end
     end
   end
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index 8d81ed5856e..c0c309d8179 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -23,29 +23,27 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
     before { project.add_developer(user) }
 
     context 'without failed checks' do
-      it "doesn't return any error" do
-        expect(subject.status).to be(true)
+      it "doesn't raise an error" do
+        expect { subject }.not_to raise_error
       end
     end
 
     context 'when the user is not allowed to push code' do
-      it 'returns an error' do
+      it 'raises an error' do
         expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
 
-        expect(subject.status).to be(false)
-        expect(subject.message).to eq('You are not allowed to push code to this project.')
+        expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
       end
     end
 
     context 'tags check' do
       let(:ref) { 'refs/tags/v1.0.0' }
 
-      it 'returns an error if the user is not allowed to update tags' do
+      it 'raises an error if the user is not allowed to update tags' do
         allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
         expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
 
-        expect(subject.status).to be(false)
-        expect(subject.message).to eq('You are not allowed to change existing tags on this project.')
+        expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
       end
 
       context 'with protected tag' do
@@ -59,8 +57,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
             let(:newrev) { '0000000000000000000000000000000000000000' }
 
             it 'is prevented' do
-              expect(subject.status).to be(false)
-              expect(subject.message).to include('cannot be deleted')
+              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/)
             end
           end
 
@@ -69,8 +66,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
             let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
 
             it 'is prevented' do
-              expect(subject.status).to be(false)
-              expect(subject.message).to include('cannot be updated')
+              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/)
             end
           end
         end
@@ -81,15 +77,14 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
           let(:ref) { 'refs/tags/v9.1.0' }
 
           it 'prevents creation below access level' do
-            expect(subject.status).to be(false)
-            expect(subject.message).to include('allowed to create this tag as it is protected')
+            expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/)
           end
 
           context 'when user has access' do
             let!(:protected_tag) { create(:protected_tag, :developers_can_create, project: project, name: 'v*') }
 
             it 'allows tag creation' do
-              expect(subject.status).to be(true)
+              expect { subject }.not_to raise_error
             end
           end
         end
@@ -101,9 +96,8 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
         let(:newrev) { '0000000000000000000000000000000000000000' }
         let(:ref) { 'refs/heads/master' }
 
-        it 'returns an error' do
-          expect(subject.status).to be(false)
-          expect(subject.message).to eq('The default branch of a project cannot be deleted.')
+        it 'raises an error' do
+          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.')
         end
       end
 
@@ -113,27 +107,24 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
           allow(ProtectedBranch).to receive(:protected?).with(project, 'feature').and_return(true)
         end
 
-        it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
+        it 'raises an error if the user is not allowed to do forced pushes to protected branches' do
           expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
 
-          expect(subject.status).to be(false)
-          expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.')
+          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.')
         end
 
-        it 'returns an error if the user is not allowed to merge to protected branches' do
+        it 'raises an error if the user is not allowed to merge to protected branches' do
           expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true)
           expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
           expect(user_access).to receive(:can_push_to_branch?).and_return(false)
 
-          expect(subject.status).to be(false)
-          expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.')
+          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.')
         end
 
-        it 'returns an error if the user is not allowed to push to protected branches' do
+        it 'raises an error if the user is not allowed to push to protected branches' do
           expect(user_access).to receive(:can_push_to_branch?).and_return(false)
 
-          expect(subject.status).to be(false)
-          expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.')
+          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.')
         end
 
         context 'branch deletion' do
@@ -141,9 +132,8 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
           let(:ref) { 'refs/heads/feature' }
 
           context 'if the user is not allowed to delete protected branches' do
-            it 'returns an error' do
-              expect(subject.status).to be(false)
-              expect(subject.message).to eq('You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
+            it 'raises an error' do
+              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
             end
           end
 
@@ -156,14 +146,13 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
               let(:protocol) { 'web' }
 
               it 'allows branch deletion' do
-                expect(subject.status).to be(true)
+                expect { subject }.not_to raise_error
               end
             end
 
             context 'over SSH or HTTP' do
-              it 'returns an error' do
-                expect(subject.status).to be(false)
-                expect(subject.message).to eq('You can only delete protected branches using the web interface.')
+              it 'raises an error' do
+                expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.')
               end
             end
           end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 0efe15856fc..10a7222c2b6 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -18,8 +18,7 @@ describe Gitlab::GitAccess, lib: true do
 
   describe '#check with single protocols allowed' do
     def disable_protocol(protocol)
-      settings = ::ApplicationSetting.create_from_defaults
-      settings.update_attribute(:enabled_git_access_protocol, protocol)
+      allow(Gitlab::ProtocolAccess).to receive(:allowed?).with(protocol).and_return(false)
     end
 
     context 'ssh disabled' do
@@ -28,11 +27,11 @@ describe Gitlab::GitAccess, lib: true do
       end
 
       it 'blocks ssh git push' do
-        expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey
+        expect { push_access_check }.to raise_unauthorized('Git access over SSH is not allowed')
       end
 
       it 'blocks ssh git pull' do
-        expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey
+        expect { pull_access_check }.to raise_unauthorized('Git access over SSH is not allowed')
       end
     end
 
@@ -44,11 +43,11 @@ describe Gitlab::GitAccess, lib: true do
       end
 
       it 'blocks http push' do
-        expect(access.check('git-receive-pack', '_any').allowed?).to be_falsey
+        expect { push_access_check }.to raise_unauthorized('Git access over HTTP is not allowed')
       end
 
       it 'blocks http git pull' do
-        expect(access.check('git-upload-pack', '_any').allowed?).to be_falsey
+        expect { pull_access_check }.to raise_unauthorized('Git access over HTTP is not allowed')
       end
     end
   end
@@ -64,23 +63,21 @@ describe Gitlab::GitAccess, lib: true do
             before { deploy_key.projects << project }
 
             it 'allows pull access' do
-              expect(pull_access_check.allowed?).to be_truthy
+              expect { pull_access_check }.not_to raise_error
             end
 
             it 'allows push access' do
-              expect(push_access_check.allowed?).to be_truthy
+              expect { push_access_check }.not_to raise_error
             end
           end
 
           context 'when the Deploykey does not have access to the project' do
             it 'blocks pulls with "not found"' do
-              expect(pull_access_check.allowed?).to be_falsey
-              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+              expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.')
             end
 
             it 'blocks pushes with "not found"' do
-              expect(push_access_check.allowed?).to be_falsey
-              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+              expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.')
             end
           end
         end
@@ -90,23 +87,21 @@ describe Gitlab::GitAccess, lib: true do
             before { project.team << [user, :master] }
 
             it 'allows pull access' do
-              expect(pull_access_check.allowed?).to be_truthy
+              expect { pull_access_check }.not_to raise_error
             end
 
             it 'allows push access' do
-              expect(push_access_check.allowed?).to be_truthy
+              expect { push_access_check }.not_to raise_error
             end
           end
 
           context 'when the User cannot read the project' do
             it 'blocks pulls with "not found"' do
-              expect(pull_access_check.allowed?).to be_falsey
-              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+              expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.')
             end
 
             it 'blocks pushes with "not found"' do
-              expect(push_access_check.allowed?).to be_falsey
-              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+              expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.')
             end
           end
         end
@@ -117,12 +112,11 @@ describe Gitlab::GitAccess, lib: true do
           let(:authentication_abilities) { build_authentication_abilities }
 
           it 'allows pull access' do
-            expect(pull_access_check.allowed?).to be_truthy
+            expect { pull_access_check }.not_to raise_error
           end
 
           it 'does not block pushes with "not found"' do
-            expect(push_access_check.allowed?).to be_falsey
-            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+            expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.')
           end
         end
       end
@@ -134,24 +128,21 @@ describe Gitlab::GitAccess, lib: true do
           let(:project) { create(:project, :repository, :public) }
 
           it 'allows pull access' do
-            expect(pull_access_check.allowed?).to be_truthy
+            expect { pull_access_check }.not_to raise_error
           end
 
           it 'does not block pushes with "not found"' do
-            expect(push_access_check.allowed?).to be_falsey
-            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+            expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.')
           end
         end
 
         context 'when guests cannot read the project' do
           it 'blocks pulls with "not found"' do
-            expect(pull_access_check.allowed?).to be_falsey
-            expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+            expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.')
           end
 
           it 'blocks pushes with "not found"' do
-            expect(push_access_check.allowed?).to be_falsey
-            expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+            expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.')
           end
         end
       end
@@ -161,10 +152,8 @@ describe Gitlab::GitAccess, lib: true do
       let(:project) { nil }
 
       it 'blocks any command with "not found"' do
-        expect(pull_access_check.allowed?).to be_falsey
-        expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
-        expect(push_access_check.allowed?).to be_falsey
-        expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+        expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.')
+        expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.')
       end
     end
   end
@@ -181,13 +170,11 @@ describe Gitlab::GitAccess, lib: true do
         end
 
         context 'when calling git-upload-pack' do
-          subject { access.check('git-upload-pack', '_any') }
-          it { expect(subject.allowed?).to be_falsey }
-          it { expect(subject.message).to eq('The command "git-upload-pack" is not allowed.') }
+          it { expect { pull_access_check }.to raise_unauthorized('The command "git-upload-pack" is not allowed.') }
         end
 
         context 'when calling git-receive-pack' do
-          it { expect(access.check('git-receive-pack', '_any').allowed?).to be_truthy }
+          it { expect { push_access_check }.not_to raise_error }
         end
       end
 
@@ -197,26 +184,22 @@ describe Gitlab::GitAccess, lib: true do
         end
 
         context 'when calling git-receive-pack' do
-          subject { access.check('git-receive-pack', '_any') }
-          it { expect(subject.allowed?).to be_falsey }
-          it { expect(subject.message).to eq('The command "git-receive-pack" is not allowed.') }
+          it { expect { push_access_check }.to raise_unauthorized('The command "git-receive-pack" is not allowed.') }
         end
 
         context 'when calling git-upload-pack' do
-          it { expect(access.check('git-upload-pack', '_any').allowed?).to be_truthy }
+          it { expect { pull_access_check }.not_to raise_error }
         end
       end
     end
   end
 
   describe '#check_download_access!' do
-    subject { access.check('git-upload-pack', '_any') }
-
     describe 'master permissions' do
       before { project.team << [user, :master] }
 
       context 'pull code' do
-        it { expect(subject.allowed?).to be_truthy }
+        it { expect { pull_access_check }.not_to raise_error }
       end
     end
 
@@ -224,8 +207,7 @@ describe Gitlab::GitAccess, lib: true do
       before { project.team << [user, :guest] }
 
       context 'pull code' do
-        it { expect(subject.allowed?).to be_falsey }
-        it { expect(subject.message).to match(/You are not allowed to download code/) }
+        it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') }
       end
     end
 
@@ -236,24 +218,22 @@ describe Gitlab::GitAccess, lib: true do
       end
 
       context 'pull code' do
-        it { expect(subject.allowed?).to be_falsey }
-        it { expect(subject.message).to match(/Your account has been blocked/) }
+        it { expect { pull_access_check }.to raise_unauthorized('Your account has been blocked.') }
       end
     end
 
     describe 'without access to project' do
       context 'pull code' do
-        it { expect(subject.allowed?).to be_falsey }
+        it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') }
       end
 
       context 'when project is public' do
         let(:public_project) { create(:project, :public, :repository) }
-        let(:guest_access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) }
-        subject { guest_access.check('git-upload-pack', '_any') }
+        let(:access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) }
 
         context 'when repository is enabled' do
           it 'give access to download code' do
-            expect(subject.allowed?).to be_truthy
+            expect { pull_access_check }.not_to raise_error
           end
         end
 
@@ -261,8 +241,7 @@ describe Gitlab::GitAccess, lib: true do
           it 'does not give access to download code' do
             public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
 
-            expect(subject.allowed?).to be_falsey
-            expect(subject.message).to match(/You are not allowed to download code/)
+            expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.')
           end
         end
       end
@@ -276,26 +255,26 @@ describe Gitlab::GitAccess, lib: true do
         context 'when project is authorized' do
           before { key.projects << project }
 
-          it { expect(subject).to be_allowed }
+          it { expect { pull_access_check }.not_to raise_error }
         end
 
         context 'when unauthorized' do
           context 'from public project' do
             let(:project) { create(:project, :public, :repository) }
 
-            it { expect(subject).to be_allowed }
+            it { expect { pull_access_check }.not_to raise_error }
           end
 
           context 'from internal project' do
             let(:project) { create(:project, :internal, :repository) }
 
-            it { expect(subject).not_to be_allowed }
+            it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') }
           end
 
           context 'from private project' do
             let(:project) { create(:project, :private, :repository) }
 
-            it { expect(subject).not_to be_allowed }
+            it { expect { pull_access_check }.to raise_not_found('The project you were looking for could not be found.') }
           end
         end
       end
@@ -308,7 +287,7 @@ describe Gitlab::GitAccess, lib: true do
         let(:project) { create(:project, :repository, namespace: user.namespace) }
 
         context 'pull code' do
-          it { expect(subject).to be_allowed }
+          it { expect { pull_access_check }.not_to raise_error }
         end
       end
 
@@ -316,7 +295,7 @@ describe Gitlab::GitAccess, lib: true do
         before { project.team << [user, :reporter] }
 
         context 'pull code' do
-          it { expect(subject).to be_allowed }
+          it { expect { pull_access_check }.not_to raise_error }
         end
       end
 
@@ -327,13 +306,13 @@ describe Gitlab::GitAccess, lib: true do
           before { project.team << [user, :reporter] }
 
           context 'pull code' do
-            it { expect(subject).to be_allowed }
+            it { expect { pull_access_check }.not_to raise_error }
           end
         end
 
         context 'when is not member of the project' do
           context 'pull code' do
-            it { expect(subject).not_to be_allowed }
+            it { expect { pull_access_check }.to raise_unauthorized('You are not allowed to download code from this project.') }
           end
         end
       end
@@ -342,7 +321,7 @@ describe Gitlab::GitAccess, lib: true do
         let(:actor) { :ci }
 
         context 'pull code' do
-          it { expect(subject).to be_allowed }
+          it { expect { pull_access_check }.not_to raise_error }
         end
       end
     end
@@ -532,42 +511,32 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  shared_examples 'pushing code' do |can|
-    subject { access.check('git-receive-pack', '_any') }
+  describe 'build authentication abilities' do
+    let(:authentication_abilities) { build_authentication_abilities }
 
     context 'when project is authorized' do
-      before { authorize }
+      before { project.team << [user, :reporter] }
 
-      it { expect(subject).public_send(can, be_allowed) }
+      it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') }
     end
 
     context 'when unauthorized' do
       context 'to public project' do
         let(:project) { create(:project, :public, :repository) }
 
-        it { expect(subject).not_to be_allowed }
+        it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') }
       end
 
       context 'to internal project' do
         let(:project) { create(:project, :internal, :repository) }
 
-        it { expect(subject).not_to be_allowed }
+        it { expect { push_access_check }.to raise_unauthorized('You are not allowed to upload code for this project.') }
       end
 
       context 'to private project' do
         let(:project) { create(:project, :private, :repository) }
 
-        it { expect(subject).not_to be_allowed }
-      end
-    end
-  end
-
-  describe 'build authentication abilities' do
-    let(:authentication_abilities) { build_authentication_abilities }
-
-    it_behaves_like 'pushing code', :not_to do
-      def authorize
-        project.team << [user, :reporter]
+        it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') }
       end
     end
   end
@@ -579,9 +548,29 @@ describe Gitlab::GitAccess, lib: true do
     context 'when deploy_key can push' do
       let(:can_push) { true }
 
-      it_behaves_like 'pushing code', :to do
-        def authorize
-          key.projects << project
+      context 'when project is authorized' do
+        before { key.projects << project }
+
+        it { expect { push_access_check }.not_to raise_error }
+      end
+
+      context 'when unauthorized' do
+        context 'to public project' do
+          let(:project) { create(:project, :public, :repository) }
+
+          it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') }
+        end
+
+        context 'to internal project' do
+          let(:project) { create(:project, :internal, :repository) }
+
+          it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') }
+        end
+
+        context 'to private project' do
+          let(:project) { create(:project, :private, :repository) }
+
+          it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') }
         end
       end
     end
@@ -589,9 +578,29 @@ describe Gitlab::GitAccess, lib: true do
     context 'when deploy_key cannot push' do
       let(:can_push) { false }
 
-      it_behaves_like 'pushing code', :not_to do
-        def authorize
-          key.projects << project
+      context 'when project is authorized' do
+        before { key.projects << project }
+
+        it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') }
+      end
+
+      context 'when unauthorized' do
+        context 'to public project' do
+          let(:project) { create(:project, :public, :repository) }
+
+          it { expect { push_access_check }.to raise_unauthorized('This deploy key does not have write access to this project.') }
+        end
+
+        context 'to internal project' do
+          let(:project) { create(:project, :internal, :repository) }
+
+          it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') }
+        end
+
+        context 'to private project' do
+          let(:project) { create(:project, :private, :repository) }
+
+          it { expect { push_access_check }.to raise_not_found('The project you were looking for could not be found.') }
         end
       end
     end
@@ -599,6 +608,14 @@ describe Gitlab::GitAccess, lib: true do
 
   private
 
+  def raise_unauthorized(message)
+    raise_error(Gitlab::GitAccess::UnauthorizedError, message)
+  end
+
+  def raise_not_found(message)
+    raise_error(Gitlab::GitAccess::NotFoundError, message)
+  end
+
   def build_authentication_abilities
     [
       :read_project,
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 1ae293416e4..a1eb95750ba 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -20,7 +20,7 @@ describe Gitlab::GitAccessWiki, lib: true do
 
     subject { access.check('git-receive-pack', changes) }
 
-    it { expect(subject.allowed?).to be_truthy }
+    it { expect { subject }.not_to raise_error }
   end
 
   def changes
@@ -36,7 +36,7 @@ describe Gitlab::GitAccessWiki, lib: true do
 
     context 'when wiki feature is enabled' do
       it 'give access to download wiki code' do
-        expect(subject.allowed?).to be_truthy
+        expect { subject }.not_to raise_error
       end
     end
 
@@ -44,8 +44,7 @@ describe Gitlab::GitAccessWiki, lib: true do
       it 'does not give access to download wiki code' do
         project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED)
 
-        expect(subject.allowed?).to be_falsey
-        expect(subject.message).to match(/You are not allowed to download code/)
+        expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to download code from this project.')
       end
     end
   end

From e8972c11904c31fc614a31483098814adc38c2ac Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 22 May 2017 11:28:51 -0700
Subject: [PATCH 09/16] Clarify error messages

And refactor to self-document a little better.
---
 lib/gitlab/git_access.rb           | 34 +++++++++++++++++++++++-------
 spec/lib/gitlab/git_access_spec.rb |  4 ++--
 spec/requests/git_http_spec.rb     |  6 +++---
 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
 

From 7d469cf1c1f356d950abc58ecbe6aa4ec15bd72b Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 22 May 2017 11:48:25 -0700
Subject: [PATCH 10/16] Fix would-be regression

https://gitlab.com/gitlab-org/gitlab-ce/commit/57e3e942de1adef2c8621905370f07d7da7870c4

I changed it to a separate condition rather than depending on the order of the case-when statements to prevent this mistake again.
---
 lib/gitlab/git_access.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 176b0991971..75cc69c02f7 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -243,9 +243,7 @@ module Gitlab
         when User
           actor
         when Key
-          actor.user
-        when DeployKey
-          nil
+          actor.user unless actor.is_a?(DeployKey)
         when :ci
           nil
         end

From 0a0f66c816469e197237a6eeaedeb959b5d1c823 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 22 May 2017 12:44:59 -0700
Subject: [PATCH 11/16] Refactor to remove a special case

---
 lib/gitlab/ci_access.rb           |  9 +++++++++
 lib/gitlab/git_access.rb          | 12 +++++-------
 spec/lib/gitlab/ci_access_spec.rb | 15 +++++++++++++++
 3 files changed, 29 insertions(+), 7 deletions(-)
 create mode 100644 lib/gitlab/ci_access.rb
 create mode 100644 spec/lib/gitlab/ci_access_spec.rb

diff --git a/lib/gitlab/ci_access.rb b/lib/gitlab/ci_access.rb
new file mode 100644
index 00000000000..def1373d8cf
--- /dev/null
+++ b/lib/gitlab/ci_access.rb
@@ -0,0 +1,9 @@
+module Gitlab
+  # For backwards compatibility, generic CI (which is a build without a user) is
+  # allowed to :build_download_code without any other checks.
+  class CiAccess
+    def can_do_action?(action)
+      action == :build_download_code
+    end
+  end
+end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 75cc69c02f7..f44426d62b8 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -29,7 +29,11 @@ module Gitlab
       @project  = project
       @protocol = protocol
       @authentication_abilities = authentication_abilities
-      @user_access = UserAccess.new(user, project: project)
+      @user_access = if ci?
+                       CiAccess.new
+                     else
+                       UserAccess.new(user, project: project)
+                     end
     end
 
     def check(cmd, changes)
@@ -62,11 +66,6 @@ module Gitlab
       authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
     end
 
-    # Allow generic CI (build without a user) for backwards compatibility
-    def ci_can_download_code?
-      authentication_abilities.include?(:build_download_code) && ci?
-    end
-
     def protocol_allowed?
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
@@ -129,7 +128,6 @@ module Gitlab
       return if deploy_key?
 
       passed = user_can_download_code? ||
-        ci_can_download_code? ||
         build_can_download_code? ||
         guest_can_download_code?
 
diff --git a/spec/lib/gitlab/ci_access_spec.rb b/spec/lib/gitlab/ci_access_spec.rb
new file mode 100644
index 00000000000..eaf8f1d0f1c
--- /dev/null
+++ b/spec/lib/gitlab/ci_access_spec.rb
@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+describe Gitlab::CiAccess, lib: true do
+  let(:access) { Gitlab::CiAccess.new }
+
+  describe '#can_do_action?' do
+    context 'when action is :build_download_code' do
+      it { expect(access.can_do_action?(:build_download_code)).to be_truthy }
+    end
+
+    context 'when action is not :build_download_code' do
+      it { expect(access.can_do_action?(:download_code)).to be_falsey }
+    end
+  end
+end

From b50a22894d4503cf99cecb8162696f854e1b3f85 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 23 May 2017 11:34:58 -0700
Subject: [PATCH 12/16] Refactor construction of response

---
 lib/api/helpers/internal_helpers.rb | 16 ++++++++++++++++
 lib/api/internal.rb                 | 20 ++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index 264df7271a3..d3732d67622 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -42,6 +42,22 @@ module API
           @project, @wiki = Gitlab::RepoPath.parse(params[:project])
         end
       end
+
+      # Project id to pass between components that don't share/don't have
+      # access to the same filesystem mounts
+      def gl_repository
+        Gitlab::GlRepository.gl_repository(project, wiki?)
+      end
+
+      # Return the repository full path so that gitlab-shell has it when
+      # handling ssh commands
+      def repository_path
+        if wiki?
+          project.wiki.repository.path_to_repo
+        else
+          project.repository.path_to_repo
+        end
+      end
     end
   end
 end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 3d60d1da114..f2d41754afd 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -37,26 +37,18 @@ module API
           .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
 
         begin
-          access_status = access_checker.check(params[:action], params[:changes])
-          response = { status: access_status.status, message: access_status.message }
+          access_checker.check(params[:action], params[:changes])
         rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
           return { status: false, message: e.message }
         end
 
         log_user_activity(actor)
 
-        # Project id to pass between components that don't share/don't have
-        # access to the same filesystem mounts
-        response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?)
-
-        # Return the repository full path so that gitlab-shell has it when
-        # handling ssh commands
-        response[:repository_path] =
-          if wiki?
-            project.wiki.repository.path_to_repo
-          else
-            project.repository.path_to_repo
-          end
+        response = {
+          status: true,
+          gl_repository: gl_repository,
+          repository_path: repository_path
+        }
 
         response
       end

From 0e3cfc75a3ae244571385c878d0025bdf7a7d394 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 23 May 2017 12:21:57 -0700
Subject: [PATCH 13/16] Remove GitAccessStatus (no longer needed)

---
 lib/gitlab/checks/change_access.rb |  4 ++--
 lib/gitlab/git_access.rb           | 14 ++++----------
 lib/gitlab/git_access_status.rb    | 11 -----------
 lib/gitlab/git_access_wiki.rb      |  6 +++---
 4 files changed, 9 insertions(+), 26 deletions(-)
 delete mode 100644 lib/gitlab/git_access_status.rb

diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index e9782623be5..b6805230348 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -31,13 +31,13 @@ module Gitlab
       end
 
       def exec
-        return GitAccessStatus.new(true) if skip_authorization
+        return true if skip_authorization
 
         push_checks
         branch_checks
         tag_checks
 
-        GitAccessStatus.new(true)
+        true
       end
 
       protected
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index f44426d62b8..4807dc9a5fb 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -51,7 +51,7 @@ module Gitlab
         check_push_access!(changes)
       end
 
-      build_status_object(true)
+      true
     end
 
     def guest_can_download_code?
@@ -167,11 +167,9 @@ module Gitlab
 
       # Iterate over all changes to find if user allowed all of them to be applied
       changes_list.each do |change|
-        status = check_single_change_access(change)
-        unless status.allowed?
-          # If user does not have access to make at least one change - cancel all push
-          raise UnauthorizedError, status.message
-        end
+        # If user does not have access to make at least one change, cancel all
+        # push by allowing the exception to bubble up
+        check_single_change_access(change)
       end
     end
 
@@ -246,9 +244,5 @@ module Gitlab
           nil
         end
     end
-
-    def build_status_object(status)
-      Gitlab::GitAccessStatus.new(status)
-    end
   end
 end
diff --git a/lib/gitlab/git_access_status.rb b/lib/gitlab/git_access_status.rb
deleted file mode 100644
index 94edf80c0f6..00000000000
--- a/lib/gitlab/git_access_status.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-module Gitlab
-  class GitAccessStatus
-    attr_accessor :status, :message
-    alias_method :allowed?, :status
-
-    def initialize(status, message = '')
-      @status = status
-      @message = message
-    end
-  end
-end
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 4c87482430f..1fe5155c093 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -13,11 +13,11 @@ module Gitlab
     end
 
     def check_single_change_access(change)
-      if user_access.can_do_action?(:create_wiki)
-        build_status_object(true)
-      else
+      unless user_access.can_do_action?(:create_wiki)
         raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
       end
+
+      true
     end
   end
 end

From d7eee7332b4a3eda49c07dfddc734bed25813f11 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 23 May 2017 12:26:46 -0700
Subject: [PATCH 14/16] Extract and memoize `user_access`

Because it is sometimes never used.
---
 lib/gitlab/git_access.rb | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 4807dc9a5fb..0a19d24eb20 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -22,18 +22,13 @@ module Gitlab
     PUSH_COMMANDS = %w{ git-receive-pack }.freeze
     ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
 
-    attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
+    attr_reader :actor, :project, :protocol, :authentication_abilities
 
     def initialize(actor, project, protocol, authentication_abilities:)
       @actor    = actor
       @project  = project
       @protocol = protocol
       @authentication_abilities = authentication_abilities
-      @user_access = if ci?
-                       CiAccess.new
-                     else
-                       UserAccess.new(user, project: project)
-                     end
     end
 
     def check(cmd, changes)
@@ -244,5 +239,13 @@ module Gitlab
           nil
         end
     end
+
+    def user_access
+      @user_access ||= if ci?
+                         CiAccess.new
+                       else
+                         UserAccess.new(user, project: project)
+                       end
+    end
   end
 end

From ad16e1ba8cd1c56bed2569aa789db430cb96984c Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 5 Jun 2017 08:17:08 -0700
Subject: [PATCH 15/16] Add changelog entry

---
 changelogs/unreleased/mk-fix-git-over-http-rejections.yml | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/mk-fix-git-over-http-rejections.yml

diff --git a/changelogs/unreleased/mk-fix-git-over-http-rejections.yml b/changelogs/unreleased/mk-fix-git-over-http-rejections.yml
new file mode 100644
index 00000000000..e75740e913f
--- /dev/null
+++ b/changelogs/unreleased/mk-fix-git-over-http-rejections.yml
@@ -0,0 +1,4 @@
+---
+title: Fix Git-over-HTTP error statuses and improve error messages
+merge_request: 11398
+author:

From ef4e913597611ee88cfcac226a409f39873eb676 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Mon, 5 Jun 2017 10:39:32 -0700
Subject: [PATCH 16/16] Remove unnecessary variable

---
 lib/api/internal.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index f2d41754afd..38631953014 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -44,13 +44,11 @@ module API
 
         log_user_activity(actor)
 
-        response = {
+        {
           status: true,
           gl_repository: gl_repository,
           repository_path: repository_path
         }
-
-        response
       end
 
       post "/lfs_authenticate" do