diff options
author | Robert Speicher <robert@gitlab.com> | 2017-04-10 16:32:59 +0000 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-04-13 20:04:02 +0800 |
commit | 353dea9f93ba6bfd3a9c27504034033f6232e74b (patch) | |
tree | ebf6f836cd31879a725541a0da52f5130e67e994 | |
parent | 2979a84b687998af0093cd20679effd3fbfac6f1 (diff) | |
download | gitlab-ce-353dea9f93ba6bfd3a9c27504034033f6232e74b.tar.gz |
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
-rw-r--r-- | lib/api/helpers/internal_helpers.rb | 6 | ||||
-rw-r--r-- | lib/api/internal.rb | 24 | ||||
-rw-r--r-- | lib/gitlab/checks/change_access.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/checks/force_push.rb | 12 | ||||
-rw-r--r-- | lib/gitlab/git/env.rb | 38 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 10 | ||||
-rw-r--r-- | lib/gitlab/git/rev_list.rb | 49 | ||||
-rw-r--r-- | lib/gitlab/git_access.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/git/env_spec.rb | 102 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/git/rev_list_spec.rb | 92 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 21 |
12 files changed, 286 insertions, 107 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 diff --git a/spec/lib/gitlab/git/env_spec.rb b/spec/lib/gitlab/git/env_spec.rb new file mode 100644 index 00000000000..d9df99bfe05 --- /dev/null +++ b/spec/lib/gitlab/git/env_spec.rb @@ -0,0 +1,102 @@ +require 'spec_helper' + +describe Gitlab::Git::Env do + describe "#set" do + context 'with RequestStore.store disabled' do + before do + allow(RequestStore).to receive(:active?).and_return(false) + end + + it 'does not store anything' do + described_class.set(GIT_OBJECT_DIRECTORY: 'foo') + + expect(described_class.all).to be_empty + end + end + + context 'with RequestStore.store enabled' do + before do + allow(RequestStore).to receive(:active?).and_return(true) + end + + it 'whitelist some `GIT_*` variables and stores them using RequestStore' do + described_class.set( + GIT_OBJECT_DIRECTORY: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar', + GIT_EXEC_PATH: 'baz', + PATH: '~/.bin:/bin') + + expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') + expect(described_class[:GIT_ALTERNATE_OBJECT_DIRECTORIES]).to eq('bar') + expect(described_class[:GIT_EXEC_PATH]).to be_nil + expect(described_class[:bar]).to be_nil + end + end + end + + describe "#all" do + context 'with RequestStore.store enabled' do + before do + allow(RequestStore).to receive(:active?).and_return(true) + described_class.set( + GIT_OBJECT_DIRECTORY: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar') + end + + it 'returns an env hash' do + expect(described_class.all).to eq({ + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' + }) + end + end + end + + describe "#[]" do + context 'with RequestStore.store enabled' do + before do + allow(RequestStore).to receive(:active?).and_return(true) + end + + before do + described_class.set( + GIT_OBJECT_DIRECTORY: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar') + end + + it 'returns a stored value for an existing key' do + expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') + end + + it 'returns nil for an non-existing key' do + expect(described_class[:foo]).to be_nil + end + end + end + + describe 'thread-safety' do + context 'with RequestStore.store enabled' do + before do + allow(RequestStore).to receive(:active?).and_return(true) + described_class.set(GIT_OBJECT_DIRECTORY: 'foo') + end + + it 'is thread-safe' do + another_thread = Thread.new do + described_class.set(GIT_OBJECT_DIRECTORY: 'bar') + + Thread.stop + described_class[:GIT_OBJECT_DIRECTORY] + end + + # Ensure another_thread runs first + sleep 0.1 until another_thread.stop? + + expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') + + another_thread.run + expect(another_thread.value).to eq('bar') + end + end + end +end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 7e8bb796e03..690f604db5e 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -40,6 +40,36 @@ describe Gitlab::Git::Repository, seed_helper: true do end end + describe "#rugged" do + context 'with no Git env stored' do + before do + expect(Gitlab::Git::Env).to receive(:all).and_return({}) + end + + it "whitelist some variables and pass them via the alternates keyword argument" do + expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: []) + + repository.rugged + end + end + + context 'with some Git env stored' do + before do + expect(Gitlab::Git::Env).to receive(:all).and_return({ + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar', + 'GIT_OTHER' => 'another_env' + }) + end + + it "whitelist some variables and pass them via the alternates keyword argument" do + expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: %w[foo bar]) + + repository.rugged + end + end + end + describe "#discover_default_branch" do let(:master) { 'master' } let(:feature) { 'feature' } diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index d48629a296d..78894ba9409 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -3,58 +3,54 @@ require 'spec_helper' describe Gitlab::Git::RevList, lib: true do let(:project) { create(:project, :repository) } - context "validations" do - described_class::ALLOWED_VARIABLES.each do |var| - context var do - it "accepts values starting with the project repo path" do - env = { var => "#{project.repository.path_to_repo}/objects" } - rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) - - expect(rev_list).to be_valid - end - - it "rejects values starting not with the project repo path" do - env = { var => "/some/other/path" } - rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) - - expect(rev_list).not_to be_valid - end - - it "rejects values containing the project repo path but not starting with it" do - env = { var => "/some/other/path/#{project.repository.path_to_repo}" } - rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) - - expect(rev_list).not_to be_valid - end - - it "ignores nil values" do - env = { var => nil } - rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) - - expect(rev_list).to be_valid - end - end - end + before do + expect(Gitlab::Git::Env).to receive(:all).and_return({ + GIT_OBJECT_DIRECTORY: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar' + }) end - context "#execute" do - let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } } - let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) } - - it "calls out to `popen` without environment variables if the record is invalid" do - allow(rev_list).to receive(:valid?).and_return(false) - - expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args) - - rev_list.execute + context "#new_refs" do + let(:rev_list) { Gitlab::Git::RevList.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) } + + it 'calls out to `popen`' do + expect(Gitlab::Popen).to receive(:popen).with([ + Gitlab.config.git.bin_path, + "--git-dir=#{project.repository.path_to_repo}", + 'rev-list', + 'newrev', + '--not', + '--all' + ], + nil, + { + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' + }).and_return(["sha1\nsha2", 0]) + + expect(rev_list.new_refs).to eq(%w[sha1 sha2]) end + end - it "calls out to `popen` with environment variables if the record is valid" do - allow(rev_list).to receive(:valid?).and_return(true) - - expect(Open3).to receive(:popen3).with(hash_including(env), any_args) - - rev_list.execute + context "#missed_ref" do + let(:rev_list) { Gitlab::Git::RevList.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) } + + it 'calls out to `popen`' do + expect(Gitlab::Popen).to receive(:popen).with([ + Gitlab.config.git.bin_path, + "--git-dir=#{project.repository.path_to_repo}", + 'rev-list', + '--max-count=1', + 'oldrev', + '^newrev' + ], + nil, + { + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' + }).and_return(["sha1\nsha2", 0]) + + expect(rev_list.missed_ref).to eq(%w[sha1 sha2]) end end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index eed45d37444..4be67df5a00 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -153,6 +153,22 @@ describe API::Internal, api: true do project.team << [user, :developer] end + context 'with env passed as a JSON' do + it 'sets env in RequestStore' do + expect(Gitlab::Git::Env).to receive(:set).with({ + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' + }) + + push(key, project.wiki, env: { + GIT_OBJECT_DIRECTORY: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar' + }.to_json) + + expect(response).to have_http_status(200) + end + end + context "git push with project.wiki" do it 'responds with success' do push(key, project.wiki) @@ -463,7 +479,7 @@ describe API::Internal, api: true do ) end - def push(key, project, protocol = 'ssh') + def push(key, project, protocol = 'ssh', env: nil) post( api("/internal/allowed"), changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master', @@ -471,7 +487,8 @@ describe API::Internal, api: true do project: project.repository.path_to_repo, action: 'git-receive-pack', secret_token: secret_token, - protocol: protocol + protocol: protocol, + env: env ) end |