Port of mk-fix-git-over-http-rejections to EE

app/controllers/concerns/lfs_request.rb

@@ -108,6 +108,10 @@ module LfsRequest
     @objects ||= (params[:objects] || []).to_a
   end
 
+  def has_authentication_ability?(capability)
+    (authentication_abilities || []).include?(capability)
+  end
+
   module EE
     def lfs_forbidden!
       raise NotImplementedError unless defined?(super)

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
-
-      render_ok
-    elsif receive_pack? && receive_pack_allowed?
-      render_ok
-    elsif http_blocked?
-      render_http_not_allowed
-    else
-      render_denied
-    end
+    log_user_activity if upload_pack?
+
+    render_ok
   end
 
   # POST /foo/bar.git/git-upload-pack (git pull)
   def git_upload_pack
-    if upload_pack? && upload_pack_allowed?
-      render_ok
-    else
-      render_denied
-    end
+    render_ok
   end
 
   # POST /foo/bar.git/git-receive-pack" (git push)
   def git_receive_pack
-    if receive_pack? && receive_pack_allowed?
-      render_ok
-    else
-      render_denied
-    end
+    render_ok
   end
 
   private
@@ -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_check.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 }
-
-        if access_status.status
-          log_user_activity(actor)
-
-          # Project id to pass between components that don't share/don't have
-          # access to the same filesystem mounts
-          response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?)
-
-          # Return the repository full path so that gitlab-shell has it when
-          # handling ssh commands
-          response[:repository_path] =
-            if wiki?
-              project.wiki.repository.path_to_repo
-            else
-              project.repository.path_to_repo
-            end
+        begin
+          access_checker.check(params[:action], params[:changes])
+        rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
+          return { status: false, message: e.message }
         end
 
-        response
+        log_user_activity(actor)
+
+        {
+          status: true,
+          gl_repository: gl_repository,
+          repository_path: repository_path
+        }
       end
 
       post "/lfs_authenticate" do

lib/gitlab/checks/change_access.rb

@@ -3,6 +3,20 @@ module Gitlab
     class ChangeAccess
       include PathLocksHelper
 
+      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
+
       # protocol is currently used only in EE
       attr_reader :user_access, :project, :skip_authorization, :protocol
 
@@ -20,22 +34,21 @@ 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_rule_check
+        push_checks
+        branch_checks
+        tag_checks
+        push_rule_check
 
-        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
 
@@ -43,7 +56,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
@@ -53,7 +66,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?
@@ -65,22 +78,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
@@ -89,7 +102,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
@@ -98,11 +111,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
 
@@ -136,7 +149,7 @@ module Gitlab
         # Prevent tag removal
         if @tag_name
           if tag_deletion_denied_by_push_rule?(push_rule)
-            return 'You cannot delete a tag'
+            raise GitAccess::UnauthorizedError, 'You cannot delete a tag'
           end
         else
           commit_validation = push_rule.try(:commit_validation?)
@@ -147,16 +160,14 @@ module Gitlab
           commits.each do |commit|
             if commit_validation
               error = check_commit(commit, push_rule)
-              return error if error
+              raise GitAccess::UnauthorizedError, error if error
             end
 
             if error = check_commit_diff(commit, push_rule)
-              return error
+              raise GitAccess::UnauthorizedError, error
             end
           end
         end
-
-        nil
       end
 
       def tag_deletion_denied_by_push_rule?(push_rule)
@@ -167,7 +178,7 @@ module Gitlab
       end
 
       # If commit does not pass push rule validation the whole push should be rejected.
-      # This method should return nil if no error found or status object if there are some errors.
+      # This method should return nil if no error found or a string if error.
       # In case of errors - all other checks will be canceled and push will be rejected.
       def check_commit(commit, push_rule)
         unless push_rule.commit_message_allowed?(commit.safe_message)

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

@@ -5,33 +5,40 @@ module Gitlab
     include ActionView::Helpers::SanitizeHelper
     include PathLocksHelper
     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.',
+      cannot_push_to_secondary_geo: "You can't push code to a secondary GitLab Geo node."
     }.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!
 
@@ -44,9 +51,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?
@@ -77,19 +82,39 @@ module Gitlab
       return if deploy_key? || geo_node_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
 
@@ -124,7 +149,7 @@ module Gitlab
       end
 
       if Gitlab::Geo.secondary?
-        raise UnauthorizedError, "You can't push code on a secondary GitLab Geo node."
+        raise UnauthorizedError, ERROR_MESSAGES[:cannot_push_to_secondary_geo]
       end
 
       if deploy_key
@@ -168,12 +193,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)
 
         if project.size_limit_enabled?
           push_size_in_bytes += EE::Gitlab::Deltas.delta_size_check(change, project.repository)
@@ -211,6 +233,10 @@ module Gitlab
       actor.is_a?(GeoNodeKey)
     end
 
+    def ci?
+      actor == :ci
+    end
+
     def can_read_project?
       if deploy_key?
         deploy_key.has_access_to?(project)
@@ -218,9 +244,31 @@ module Gitlab
         true
       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
@@ -230,17 +278,21 @@ module Gitlab
         case actor
         when User
           actor
-        when DeployKey
-          nil
         when GeoNodeKey
           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,13 +13,11 @@ module Gitlab
     end
 
     def check_single_change_access(change)
-      if Gitlab::Geo.enabled? && Gitlab::Geo.secondary?
-        build_status_object(false, "You can't push code to a secondary GitLab Geo node.")
-      elsif 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/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_wiki_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::GitAccessWiki, lib: true do
   let(:access) { Gitlab::GitAccessWiki.new(user, project, 'web', authentication_abilities: authentication_abilities) }
-  let(:project) { create(:project, :repository) }
+  let!(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master'] }
   let(:authentication_abilities) do
@@ -22,15 +22,18 @@ 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 }
 
       context 'when in a secondary gitlab geo node' do
         before do
           allow(Gitlab::Geo).to receive(:enabled?) { true }
           allow(Gitlab::Geo).to receive(:secondary?) { true }
+          allow(Gitlab::Geo).to receive(:license_allows?) { true }
         end
 
-        it { expect(subject.allowed?).to be_falsey }
+        it 'does not give access to upload wiki code' do
+          expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "You can't push code to a secondary GitLab Geo node.")
+        end
       end
     end
   end
@@ -44,7 +47,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
 
@@ -52,8 +55,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/lfs_http_spec.rb

@@ -795,8 +795,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
 
@@ -805,8 +805,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
@@ -814,8 +815,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
@@ -1026,8 +1027,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
 
@@ -1040,8 +1041,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
@@ -1053,8 +1054,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
@@ -1126,8 +1127,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
 
@@ -1136,8 +1137,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
@@ -1145,8 +1147,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