Fix Git over HTTP spec

* 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

 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
 

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
 

lib/gitlab/git_access_wiki.rb

 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

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 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 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 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 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
+  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 "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
+      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 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 the project doesn't exist" do
+      let(:path) { 'doesnt/exist.git' }
 
-        download("/#{wiki.repository.path_with_namespace}.git") do |response|
-          json_body = ActiveSupport::JSON.decode(response.body)
+      it_behaves_like 'pulls require Basic HTTP Authentication'
+      it_behaves_like 'pushes require Basic HTTP Authentication'
 
-          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)
+      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 requesting the Wiki" do
+      let(:wiki) { ProjectWiki.new(project) }
+      let(:path) { "/#{wiki.repository.path_with_namespace}.git" }
+
+      context "when the project is public" do
+        let(:project) { create(:project, :repository, :public, :wiki_enabled) }
+
+        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]
+          it_behaves_like 'pulls are allowed'
+
+          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 clones' do
-          download(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
 
-        it 'allows pushes' do
-          upload(path, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+      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)
-        end
+        let(:project) { create(:project, :repository, :public) }
 
-        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_behaves_like 'pushes require Basic HTTP Authentication'
 
-        it "uploads get status 401" do
-          upload(path, {}) do |response|
-            expect(response).to have_http_status(401)
-          end
+        context 'when not authenticated' do
+          let(:env) { {} }
+
+          it_behaves_like 'pulls are allowed'
         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)
-
-                  clone_get(path, env)
+                it "resets the IP in Rack Attack on download" do
+                  expect(Rack::Attack::Allow2Ban).to receive(:reset).twice
 
-                  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
-
-                  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
+                let(:path) { "#{project.path_with_namespace}.git" }
+                let(:env) { { 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)
-            end
+            it_behaves_like 'pulls are allowed'
 
-            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
+            it "rejects pushes with 401 Unauthorized (no project existence information leak)" do
+              push_get(path, env)
 
-              expect(response).to have_http_status(401)
+              expect(response).to have_http_status(:unauthorized)
             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
+            # 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(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 "GET info/refs" do
+          let(:path) { "/#{project.path_with_namespace}/info/refs" }
 
-        context "when no params are added" do
-          before { get path }
+          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")
+            it "redirects to the .git suffix version" do
+              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs")
+            end
           end
-        end
 
-        context "when the upload-pack service is requested" do
-          let(:params) { { service: 'git-upload-pack' } }
-          before { get path, params }
+          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]}")
+            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
-        end
 
-        context "when the receive-pack service is requested" do
-          let(:params) { { service: 'git-receive-pack' } }
-          before { get path, params }
+          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]}")
+            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
-        end
 
-        context "when the params are anything else" do
-          let(:params) { { service: 'git-implode-pack' } }
-          before { get path, params }
+          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)
+            it "redirects to the sign-in page" do
+              expect(response).to redirect_to(new_user_session_path)
+            end
           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)
+        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
-      end
 
-      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)
+        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
-    end
 
-    context "retrieving an info/refs file" do
-      before { project.update_attribute(:visibility_level, Project::PUBLIC) }
+      context "retrieving an info/refs file" do
+        let(:project) { create(:project, :repository, :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')
+            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')
+            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
+        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, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+          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

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