Merge branch 'backport-ee-1525' into 'master'

Backport Git-env code from EE "Fix push rules on Git 2.11" (gitlab-org/gitlab-ee!1525)

See merge request !10547

8 files changed, 91 insertions, 57 deletions

diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index 2135a787b11..810e5063996 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -53,12 +53,12 @@ module API
         ]
       end
 
-      def parse_allowed_environment_variables
-        return if params[:env].blank?
+      def parse_env
+        return {} if params[:env].blank?
 
         JSON.parse(params[:env])
-
       rescue JSON::ParserError
+        {}
       end
     end
   end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 70d0d57204d..215bc03d0e9 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -11,14 +11,16 @@ module API
       # Params:
       #   key_id - ssh key id for Git over SSH
       #   user_id - user id for Git over HTTP
+      #   protocol - Git access protocol being used, e.g. HTTP or SSH
       #   project - project path with namespace
       #   action - git action (git-upload-pack or git-receive-pack)
-      #   ref - branch name
-      #   forced_push - forced_push
-      #   protocol - Git access protocol being used, e.g. HTTP or SSH
+      #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
       post "/allowed" do
         status 200
 
+        # Stores some Git-specific env thread-safely
+        Gitlab::Git::Env.set(parse_env)
+
         actor =
           if params[:key_id]
             Key.find_by(id: params[:key_id])
@@ -30,18 +32,10 @@ module API
 
         actor.update_last_used_at if actor.is_a?(Key)
 
-        access =
-          if wiki?
-            Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
-          else
-            Gitlab::GitAccess.new(actor,
-                                  project,
-                                  protocol,
-                                  authentication_abilities: ssh_authentication_abilities,
-                                  env: parse_allowed_environment_variables)
-          end
-
-        access_status = access.check(params[:action], params[:changes])
+        access_checker = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
+        access_status = access_checker
+          .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
+          .check(params[:action], params[:changes])
 
         response = { status: access_status.status, message: access_status.message }
 
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index eb2f2e144fd..8793b20aa35 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -5,7 +5,7 @@ module Gitlab
       attr_reader :user_access, :project, :skip_authorization, :protocol
 
       def initialize(
-        change, user_access:, project:, env: {}, skip_authorization: false,
+        change, user_access:, project:, skip_authorization: false,
         protocol:
       )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@@ -13,7 +13,6 @@ module Gitlab
         @tag_name = Gitlab::Git.tag_name(@ref)
         @user_access = user_access
         @project = project
-        @env = env
         @skip_authorization = skip_authorization
         @protocol = protocol
       end
@@ -97,7 +96,7 @@ module Gitlab
       end
 
       def forced_push?
-        Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env)
+        Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
       end
 
       def update?
diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb
index de0c9049ebf..1e73f89158d 100644
--- a/lib/gitlab/checks/force_push.rb
+++ b/lib/gitlab/checks/force_push.rb
@@ -1,20 +1,16 @@
 module Gitlab
   module Checks
     class ForcePush
-      def self.force_push?(project, oldrev, newrev, env: {})
+      def self.force_push?(project, oldrev, newrev)
         return false if project.empty_repo?
 
         # Created or deleted branch
         if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
           false
         else
-          missed_ref, exit_status = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute
-
-          if exit_status == 0
-            missed_ref.present?
-          else
-            raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
-          end
+          Gitlab::Git::RevList.new(
+            path_to_repo: project.repository.path_to_repo,
+            oldrev: oldrev, newrev: newrev).missed_ref.present?
         end
       end
     end
diff --git a/lib/gitlab/git/env.rb b/lib/gitlab/git/env.rb
new file mode 100644
index 00000000000..0fdc57ec954
--- /dev/null
+++ b/lib/gitlab/git/env.rb
@@ -0,0 +1,38 @@
+module Gitlab
+  module Git
+    # Ephemeral (per request) storage for environment variables that some Git
+    # commands may need.
+    #
+    # For example, in pre-receive hooks, new objects are put in a temporary
+    # $GIT_OBJECT_DIRECTORY. Without it set, the new objects cannot be retrieved
+    # (this would break push rules for instance).
+    #
+    # This class is thread-safe via RequestStore.
+    class Env
+      WHITELISTED_GIT_VARIABLES = %w[
+        GIT_OBJECT_DIRECTORY
+        GIT_ALTERNATE_OBJECT_DIRECTORIES
+      ].freeze
+
+      def self.set(env)
+        return unless RequestStore.active?
+
+        RequestStore.store[:gitlab_git_env] = whitelist_git_env(env)
+      end
+
+      def self.all
+        return {} unless RequestStore.active?
+
+        RequestStore.fetch(:gitlab_git_env) { {} }
+      end
+
+      def self.[](key)
+        all[key]
+      end
+
+      def self.whitelist_git_env(env)
+        env.select { |key, _| WHITELISTED_GIT_VARIABLES.include?(key.to_s) }.with_indifferent_access
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index fc473b2c21e..41ab73abb56 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -8,6 +8,10 @@ module Gitlab
     class Repository
       include Gitlab::Git::Popen
 
+      ALLOWED_OBJECT_DIRECTORIES_VARIABLES = %w[
+        GIT_OBJECT_DIRECTORY
+        GIT_ALTERNATE_OBJECT_DIRECTORIES
+      ].freeze
       SEARCH_CONTEXT_LINES = 3
 
       NoRepository = Class.new(StandardError)
@@ -58,7 +62,7 @@ module Gitlab
       end
 
       def rugged
-        @rugged ||= Rugged::Repository.new(path)
+        @rugged ||= Rugged::Repository.new(path, alternates: alternate_object_directories)
       rescue Rugged::RepositoryError, Rugged::OSError
         raise NoRepository.new('no repository for such path')
       end
@@ -978,6 +982,10 @@ module Gitlab
 
       private
 
+      def alternate_object_directories
+        Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES).compact
+      end
+
       # Get the content of a blob for a given commit.  If the blob is a commit
       # (for submodules) then return the blob's OID.
       def blob_content(commit, blob_name)
diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
index 79dd0cf7df2..a16b0ed76f4 100644
--- a/lib/gitlab/git/rev_list.rb
+++ b/lib/gitlab/git/rev_list.rb
@@ -1,41 +1,42 @@
 module Gitlab
   module Git
     class RevList
-      attr_reader :project, :env
-
-      ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze
-
-      def initialize(oldrev, newrev, project:, env: nil)
-        @project = project
-        @env = env.presence || {}
-        @args = [Gitlab.config.git.bin_path,
-                 "--git-dir=#{project.repository.path_to_repo}",
-                 "rev-list",
-                 "--max-count=1",
-                 oldrev,
-                 "^#{newrev}"]
+      attr_reader :oldrev, :newrev, :path_to_repo
+
+      def initialize(path_to_repo:, newrev:, oldrev: nil)
+        @oldrev = oldrev
+        @newrev = newrev
+        @path_to_repo = path_to_repo
       end
 
-      def execute
-        Gitlab::Popen.popen(@args, nil, parse_environment_variables)
+      # This method returns an array of new references
+      def new_refs
+        execute([*base_args, newrev, '--not', '--all'])
       end
 
-      def valid?
-        environment_variables.all? do |(name, value)|
-          value.to_s.start_with?(project.repository.path_to_repo)
-        end
+      # This methods returns an array of missed references
+      def missed_ref
+        execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
       end
 
       private
 
-      def parse_environment_variables
-        return {} unless valid?
+      def execute(args)
+        output, status = Gitlab::Popen.popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
+
+        unless status.zero?
+          raise "Got a non-zero exit code while calling out `#{args.join(' ')}`."
+        end
 
-        environment_variables
+        output.split("\n")
       end
 
-      def environment_variables
-        @environment_variables ||= env.slice(*ALLOWED_VARIABLES).compact
+      def base_args
+        [
+          Gitlab.config.git.bin_path,
+          "--git-dir=#{path_to_repo}",
+          'rev-list'
+        ]
       end
     end
   end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index eea2f206902..99724db8da2 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -18,13 +18,12 @@ module Gitlab
 
     attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
 
-    def initialize(actor, project, protocol, authentication_abilities:, env: {})
+    def initialize(actor, project, protocol, authentication_abilities:)
       @actor    = actor
       @project  = project
       @protocol = protocol
       @authentication_abilities = authentication_abilities
       @user_access = UserAccess.new(user, project: project)
-      @env = env
     end
 
     def check(cmd, changes)
@@ -152,7 +151,6 @@ module Gitlab
         change,
         user_access: user_access,
         project: project,
-        env: @env,
         skip_authorization: deploy_key?,
         protocol: protocol
       ).exec