Merge branch 'mk-fix-git-over-http-rejections' into 'master'

Fix Git-over-HTTP rejections

See merge request !11398

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

app/controllers/projects/git_http_client_controller.rb

@@ -128,32 +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
-
-  def authentication_project
-    authentication_result.project
-  end
 end

app/controllers/projects/git_http_controller.rb

 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
-    elsif http_blocked?
-      render_http_not_allowed
-    else
-      render_denied
-    end
   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
   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
   end
 
   private
@@ -45,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]
@@ -62,47 +47,27 @@ 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
+  def render_403(exception)
+    render plain: exception.message, status: :forbidden
   end
 
-  def render_denied
-    if user && can?(user, :read_project, project)
-      render plain: access_denied_message, status: :forbidden
-    else
-      # Do not leak information about project existence
-      render_not_found
-    end
-  end
-
-  def access_denied_message
-    'Access denied'
+  def render_404(exception)
+    render plain: exception.message, status: :not_found
   end
 
-  def upload_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.upload_pack
-
-    access_check.allowed? || ci?
+  def access
+    @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
   end
 
-  def access
-    @access ||= access_klass.new(user, project, 'http', authentication_abilities: authentication_abilities)
+  def access_actor
+    return user if user
+    return :ci if ci?
   end
 
   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 http_blocked?
-    !access.protocol_allowed?
-  end
-
-  def receive_pack_allowed?
-    return false unless Gitlab.config.gitlab_shell.receive_pack
-
-    access_check.allowed?
+    access.check(git_command, '_any')
   end
 
   def access_klass

changelogs/unreleased/mk-fix-git-over-http-rejections.yml

+---
+title: Fix Git-over-HTTP error statuses and improve error messages
+merge_request: 11398
+author:

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

lib/api/internal.rb

@@ -32,31 +32,23 @@ 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 }
+        begin
+          access_checker.check(params[:action], params[:changes])
+        rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
+          return { status: false, message: e.message }
+        end
 
-        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
-        end
-
-        response
+        {
+          status: true,
+          gl_repository: gl_repository,
+          repository_path: repository_path
+        }
       end
 
       post "/lfs_authenticate" do

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(
@@ -17,22 +31,20 @@ module Gitlab
       end
 
       def exec
-        return GitAccessStatus.new(true) if skip_authorization
+        return 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
+        true
       end
 
       protected
 
       def push_checks
         if user_access.cannot_do_action?(:push_code)
-          "You are not allowed to push code to this project."
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
         end
       end
 
@@ -40,7 +52,7 @@ module Gitlab
         return unless @branch_name
 
         if deletion? && @branch_name == project.default_branch
-          return "The default branch of a project cannot be deleted."
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:delete_default_branch]
         end
 
         protected_branch_checks
@@ -50,7 +62,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."
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:force_push_protected_branch]
         end
 
         if deletion?
@@ -62,22 +74,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.'
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_master_delete_protected_branch]
         end
 
         unless protocol == 'web'
-          'You can only delete protected branches using the web interface.'
+          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)
-            "You are not allowed to merge code into protected branches on this project."
+            raise GitAccess::UnauthorizedError, 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."
+            raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_protected_branch]
           end
         end
       end
@@ -86,7 +98,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."
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:change_existing_tags]
         end
 
         protected_tag_checks
@@ -95,11 +107,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?
+        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 "You are not allowed to create this tag as it is protected."
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:create_protected_tag]
         end
       end
 

lib/gitlab/ci_access.rb

+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

lib/gitlab/git_access.rb

@@ -3,33 +3,39 @@
 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.',
       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.",
+      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
     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 = UserAccess.new(user, project: project)
     end
 
     def check(cmd, changes)
       check_protocol!
       check_active_user!
       check_project_accessibility!
+      check_command_disabled!(cmd)
       check_command_existence!(cmd)
       check_repository_existence!
 
@@ -40,9 +46,7 @@ module Gitlab
         check_push_access!(changes)
       end
 
-      build_status_object(true)
-    rescue UnauthorizedError => ex
-      build_status_object(false, ex.message)
+      true
     end
 
     def guest_can_download_code?
@@ -73,19 +77,39 @@ 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 NotFoundError, ERROR_MESSAGES[:project_not_found]
+      end
+    end
+
+    def check_command_disabled!(cmd)
+      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
 
     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
 
@@ -138,11 +162,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
 
@@ -168,14 +190,40 @@ 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
 
+    def http?
+      protocol == 'http'
+    end
+
+    def upload_pack?(command)
+      command == 'git-upload-pack'
+    end
+
+    def receive_pack?(command)
+      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
@@ -185,15 +233,19 @@ module Gitlab
         case actor
         when User
           actor
-        when DeployKey
-          nil
         when Key
-          actor.user
+          actor.user unless actor.is_a?(DeployKey)
+        when :ci
+          nil
         end
     end
 
-    def build_status_object(status, message = '')
-      Gitlab::GitAccessStatus.new(status, message)
+    def user_access
+      @user_access ||= if ci?
+                         CiAccess.new
+                       else
+                         UserAccess.new(user, project: project)
+                       end
     end
   end
 end

lib/gitlab/git_access_status.rb

-module Gitlab
-  class GitAccessStatus
-    attr_accessor :status, :message
-    alias_method :allowed?, :status
-
-    def initialize(status, message = '')
-      @status = status
-      @message = message
-    end
-
-    def to_json(opts = nil)
-      { status: @status, message: @message }.to_json(opts)
-    end
-  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
@@ -9,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
-        build_status_object(false, "You are not allowed to write to this project's wiki.")
+      unless user_access.can_do_action?(:create_wiki)
+        raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
       end
+
+      true
     end
   end
 end

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

spec/lib/gitlab/ci_access_spec.rb

+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

spec/lib/gitlab/git_access_spec.rb

 require 'spec_helper'
 
 describe Gitlab::GitAccess, lib: true do
-  let(:access) { Gitlab::GitAccess.new(actor, project, 'ssh', authentication_abilities: authentication_abilities) }
+  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) }
   let(:actor) { user }
+  let(:protocol) { 'ssh' }
   let(:authentication_abilities) do
     [
       :read_project,
@@ -15,49 +18,188 @@ 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
       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 { push_access_check }.to raise_unauthorized('Git access over SSH is not allowed')
       end
 
       it 'blocks ssh git pull' do
-        expect(@acc.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
 
     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 { push_access_check }.to raise_unauthorized('Git access over HTTP is not allowed')
       end
 
       it 'blocks http git pull' do
-        expect(@acc.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
 
-  describe '#check_download_access!' do
-    subject { access.check('git-upload-pack', '_any') }
+  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 }.not_to raise_error
+            end
+
+            it 'allows push access' do
+              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 }.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 }.to raise_not_found('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 }.not_to raise_error
+            end
 
+            it 'allows push access' do
+              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 }.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 }.to raise_not_found('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 }.not_to raise_error
+          end
+
+          it 'does not block pushes with "not found"' do
+            expect { push_access_check }.to raise_unauthorized('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 }.not_to raise_error
+          end
+
+          it 'does not block pushes with "not found"' do
+            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 }.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 }.to raise_not_found('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 }.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
+
+  describe '#check_command_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
+          it { expect { pull_access_check }.to raise_unauthorized('Pulling over HTTP is not allowed.') }
+        end
+
+        context 'when calling git-receive-pack' do
+          it { expect { push_access_check }.not_to raise_error }
+        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
+          it { expect { push_access_check }.to raise_unauthorized('Pushing over HTTP is not allowed.') }
+        end
+
+        context 'when calling git-upload-pack' do
+          it { expect { pull_access_check }.not_to raise_error }
+        end
+      end
+    end
+  end
+
+  describe '#check_download_access!' do
     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
 
@@ -65,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
 
@@ -77,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
 
@@ -102,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
@@ -117,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
@@ -149,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
 
@@ -157,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
 
@@ -168,16 +306,24 @@ 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
+
+      describe 'generic CI (build without a user)' do
+        let(:actor) { :ci }
+
+        context 'pull code' do
+          it { expect { pull_access_check }.not_to raise_error }
+        end
+      end
     end
   end
 
@@ -365,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
@@ -412,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
@@ -422,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
@@ -432,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,

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

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
 
+    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
+
+  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 with status 401" do
-            download('doesnt/exist.git', user: user.username, password: "nope") do |response|
-              expect(response).to have_http_status(401)
+        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)
+        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
 
-    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)
+  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
 
-        download("/#{wiki.repository.path_with_namespace}.git") do |response|
+  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 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 'when unauthenticated' do
+          let(:env) { {} }
+
+          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(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
         end
 
+        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_disabled, :wiki_enabled) }
-        let(:wiki) { ProjectWiki.new(project) }
-        let(:path) { "/#{wiki.repository.path_with_namespace}.git" }
+              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(200)
+                  expect(response).to have_http_status(:ok)
                 end
               end
 
-        it 'allows pushes' do
+              it 'pushes are allowed' do
                 upload(path, user: user.username, password: user.password) do |response|
-            expect(response).to have_http_status(200)
+                  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)
-            end
+          context 'as a developer on the team' do
+            before do
+              project.team << [user, :developer]
             end
 
-          context 'but git-receive-pack is disabled' do
-            it "responds with status 404" do
+            it_behaves_like 'pulls are allowed'
+            it_behaves_like 'pushes are allowed'
+
+            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(403)
-              end
+                  expect(response).to have_http_status(:forbidden)
+                  expect(response.body).to eq(git_access_error(:receive_pack_disabled_over_http))
                 end
               end
             end
 
-        context 'but git-upload-pack is disabled' do
-          it "responds with status 404" do
+            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, {}) do |response|
-              expect(response).to have_http_status(404)
+                download(path, env) do |response|
+                  expect(response).to have_http_status(:forbidden)
+                  expect(response.body).to eq(git_access_error(:upload_pack_disabled_over_http))
+                end
+              end
+            end
+          end
+
+          context 'and not a member of 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(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)
+                  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
@@ -373,28 +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
-            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
+            # 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(401)
+              expect(response).to have_http_status(:forbidden)
+              expect(response.body).to eq(git_access_error(:upload))
             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
+            # 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(404)
+              expect(response).to have_http_status(:not_found)
+              expect(response.body).to eq(git_access_error(:project_not_found))
             end
           end
 
@@ -405,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)
-
-                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 'downloads from non-existing repository and gets 403' do
-                allow_any_instance_of(Repository).
-                  to receive(:exists?).and_return(false)
+              context 'when the repo does not exist' do
+                let(:project) { create(:empty_project) }
 
-                clone_get "#{project.path_with_namespace}.git",
-                  user: 'gitlab-ci-token', password: build.token
+                it 'rejects pulls with 403 Forbidden' do
+                  clone_get path, env
 
-                expect(response).to have_http_status(403)
+                  expect(response).to have_http_status(:forbidden)
+                  expect(response.body).to eq(git_access_error(:no_repo))
+                end
               end
 
-              it 'uploads get status 403' do
-                push_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(401)
+                expect(response).to have_http_status(:forbidden)
+                expect(response.body).to eq(git_access_error(:upload))
               end
             end
 
@@ -441,7 +566,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,8 +578,7 @@ 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)
-              end
+                expect(response).to have_http_status(:not_found)
               end
             end
           end
@@ -462,6 +586,8 @@ describe 'Git HTTP requests', lib: true do
       end
 
       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" }
 
@@ -515,7 +641,7 @@ describe 'Git HTTP requests', lib: true do
       end
 
       context "retrieving an info/refs file" do
-      before { project.update_attribute(:visibility_level, Project::PUBLIC) }
+        let(:project) { create(:project, :repository, :public) }
 
         context "when the file exists" do
           before do
@@ -529,7 +655,7 @@ describe 'Git HTTP requests', lib: true do
           end
 
           it "returns the file" do
-          expect(response).to have_http_status(200)
+            expect(response).to have_http_status(:ok)
           end
         end
 
@@ -537,7 +663,8 @@ describe 'Git HTTP requests', lib: true do
           before { get "/#{project.path_with_namespace}/blob/master/info/refs" }
 
           it "returns not found" do
-          expect(response).to have_http_status(404)
+            expect(response).to have_http_status(:not_found)
+          end
         end
       end
     end
@@ -546,6 +673,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,45 +681,37 @@ 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 } }
 
+        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|
+            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
   end

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

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,19 @@ module GitHttpHelpers
 
     env
   end
+
+  def git_access_error(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)
+    message = Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+    message || raise("GitAccessWiki error message key '#{error_key}' not found")
+  end
+
+  def change_access_error(error_key)
+    message = Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+    message || raise("ChangeAccess error message key '#{error_key}' not found")
+  end
 end