Merge branch 'gl-id-refactor' into 'master'

Refactor logic around GL_ID.

Builds upon !8.

See merge request !9

7 files changed, 47 insertions, 58 deletions

diff --git a/bin/gitlab-shell b/bin/gitlab-shell
index 8154029..084f0c9 100755
--- a/bin/gitlab-shell
+++ b/bin/gitlab-shell
@@ -5,6 +5,9 @@ unless ENV['SSH_CONNECTION']
   exit
 end
 
+key_id = /key-[0-9]+/.match(ARGV.join).to_s
+original_cmd = ENV['SSH_ORIGINAL_COMMAND']
+
 require_relative '../lib/gitlab_init'
 
 #
@@ -14,7 +17,7 @@ require_relative '../lib/gitlab_init'
 #
 require File.join(ROOT_PATH, 'lib', 'gitlab_shell')
 
-if GitlabShell.new.exec
+if GitlabShell.new(key_id, original_cmd).exec
   exit 0
 else
   exit 1
diff --git a/hooks/post-receive b/hooks/post-receive
index 301f639..da0ff41 100755
--- a/hooks/post-receive
+++ b/hooks/post-receive
@@ -7,6 +7,9 @@ refs = ARGF.read
 key_id  = ENV['GL_ID']
 repo_path = Dir.pwd
 
+# reset GL_ID env since we already got its value
+ENV['GL_ID'] = nil
+
 require_relative '../lib/gitlab_custom_hook'
 require_relative '../lib/gitlab_post_receive'
 
diff --git a/lib/gitlab_custom_hook.rb b/lib/gitlab_custom_hook.rb
index a099e79..ac6837d 100644
--- a/lib/gitlab_custom_hook.rb
+++ b/lib/gitlab_custom_hook.rb
@@ -4,24 +4,21 @@ class GitlabCustomHook
   def pre_receive(changes, repo_path)
     hook = hook_file('pre-receive', repo_path)
     return true if hook.nil?
-    if call_receive_hook(hook, changes)
-      return true
-    else
-      # reset GL_ID env since we stop git push here
-      ENV['GL_ID'] = nil
-      return false
-    end
+
+    call_receive_hook(hook, changes)
   end
 
   def post_receive(changes, repo_path)
     hook = hook_file('post-receive', repo_path)
     return true if hook.nil?
-    call_receive_hook(hook, changes) ? true : false
+    
+    call_receive_hook(hook, changes)
   end
 
   def update(ref_name, old_value, new_value, repo_path)
     hook = hook_file('update', repo_path)
     return true if hook.nil?
+
     system(hook, ref_name, old_value, new_value)
   end
 
diff --git a/lib/gitlab_post_receive.rb b/lib/gitlab_post_receive.rb
index 3c601ee..ede64f2 100644
--- a/lib/gitlab_post_receive.rb
+++ b/lib/gitlab_post_receive.rb
@@ -13,10 +13,6 @@ class GitlabPostReceive
   end
 
   def exec
-    # reset GL_ID env since we already
-    # get value from it
-    ENV['GL_ID'] = nil
-
     result = update_redis
 
     begin
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index d075048..7249836 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -7,11 +7,13 @@ class GitlabShell
   class DisallowedCommandError < StandardError; end
   class InvalidRepositoryPathError < StandardError; end
 
+  GIT_COMMANDS = %w(git-upload-pack git-receive-pack git-upload-archive git-annex-shell).freeze
+
   attr_accessor :key_id, :repo_name, :git_cmd, :repos_path, :repo_name
 
-  def initialize
-    @key_id = /key-[0-9]+/.match(ARGV.join).to_s
-    @origin_cmd = ENV['SSH_ORIGINAL_COMMAND']
+  def initialize(key_id, origin_cmd)
+    @key_id = key_id
+    @origin_cmd = origin_cmd
     @config = GitlabConfig.new
     @repos_path = @config.repos_path
   end
@@ -24,12 +26,7 @@ class GitlabShell
 
     parse_cmd
 
-    raise DisallowedCommandError unless git_cmds.include?(@git_cmd)
-
-    ENV['GL_ID'] = @key_id
-    status = api.check_access(@git_cmd, @repo_name, @key_id, '_any')
-
-    raise AccessDeniedError, status.message unless status.allowed?
+    verify_access
 
     process_cmd
 
@@ -60,6 +57,8 @@ class GitlabShell
     args = Shellwords.shellwords(@origin_cmd)
     @git_cmd = args.first
 
+    raise DisallowedCommandError unless GIT_COMMANDS.include?(@git_cmd)
+
     if @git_cmd == 'git-annex-shell'
       raise DisallowedCommandError unless @config.git_annex_enabled?
 
@@ -73,8 +72,10 @@ class GitlabShell
     end
   end
 
-  def git_cmds
-    %w(git-upload-pack git-receive-pack git-upload-archive git-annex-shell)
+  def verify_access
+    status = api.check_access(@git_cmd, @repo_name, @key_id, '_any')
+
+    raise AccessDeniedError, status.message unless status.allowed?
   end
 
   def process_cmd
@@ -105,7 +106,7 @@ class GitlabShell
 
   # This method is not covered by Rspec because it ends the current Ruby process.
   def exec_cmd(*args)
-    Kernel::exec({ 'PATH' => ENV['PATH'], 'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'], 'GL_ID' => ENV['GL_ID'] }, *args, unsetenv_others: true)
+    Kernel::exec({ 'PATH' => ENV['PATH'], 'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'], 'GL_ID' => @key_id }, *args, unsetenv_others: true)
   end
 
   def api
diff --git a/spec/gitlab_post_receive_spec.rb b/spec/gitlab_post_receive_spec.rb
index 5a9892a..3c1f362 100644
--- a/spec/gitlab_post_receive_spec.rb
+++ b/spec/gitlab_post_receive_spec.rb
@@ -24,14 +24,6 @@ describe GitlabPostReceive do
       allow(gitlab_post_receive).to receive(:system).and_return(true)
     end
 
-    it "resets the GL_ID environment variable" do
-      ENV["GL_ID"] = actor
-
-      gitlab_post_receive.exec
-
-      expect(ENV["GL_ID"]).to be_nil
-    end
-
     it "prints the broadcast message" do
       expect(gitlab_post_receive).to receive(:puts).ordered
       expect(gitlab_post_receive).to receive(:puts).with(
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 6bb4db4..77523d7 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -13,7 +13,7 @@ describe GitlabShell do
 
   subject do
     ARGV[0] = key_id
-    GitlabShell.new.tap do |shell|
+    GitlabShell.new(key_id, ssh_cmd).tap do |shell|
       shell.stub(exec_cmd: :exec_called)
       shell.stub(api: api)
     end
@@ -27,6 +27,7 @@ describe GitlabShell do
   end
 
   let(:key_id) { "key-#{rand(100) + 100}" }
+  let(:ssh_cmd) { nil }
   let(:tmp_repos_path) { File.join(ROOT_PATH, 'tmp', 'repositories') }
 
   before do
@@ -34,7 +35,7 @@ describe GitlabShell do
   end
 
   describe :initialize do
-    before { ssh_cmd 'git-receive-pack' }
+    let(:ssh_cmd) { 'git-receive-pack' }
 
     its(:key_id) { should == key_id }
     its(:repos_path) { should == tmp_repos_path }
@@ -43,8 +44,9 @@ describe GitlabShell do
   describe :parse_cmd do
     describe 'git' do
       context 'w/o namespace' do
+        let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
+
         before do
-          ssh_cmd 'git-upload-pack gitlab-ci.git'
           subject.send :parse_cmd
         end
 
@@ -53,8 +55,9 @@ describe GitlabShell do
       end
 
       context 'namespace' do
+        let(:ssh_cmd) { 'git-upload-pack dmitriy.zaporozhets/gitlab-ci.git' }
+
         before do
-          ssh_cmd 'git-upload-pack dmitriy.zaporozhets/gitlab-ci.git'
           subject.send :parse_cmd
         end
 
@@ -63,7 +66,7 @@ describe GitlabShell do
       end
 
       context 'with an invalid number of arguments' do
-        before { ssh_cmd 'foobar' }
+        let(:ssh_cmd) { 'foobar' }
 
         it "should raise an DisallowedCommandError" do
           expect { subject.send :parse_cmd }.to raise_error(GitlabShell::DisallowedCommandError)
@@ -74,6 +77,8 @@ describe GitlabShell do
     describe 'git-annex' do
       let(:repo_path) { File.join(tmp_repos_path, 'dzaporozhets/gitlab.git') }
 
+      let(:ssh_cmd) { 'git-annex-shell inannex /~/dzaporozhets/gitlab.git SHA256E' }
+
       before do
         GitlabConfig.any_instance.stub(git_annex_enabled?: true)
 
@@ -82,7 +87,6 @@ describe GitlabShell do
         cmd = %W(git --git-dir=#{repo_path} init --bare)
         system(*cmd)
 
-        ssh_cmd 'git-annex-shell inannex /~/dzaporozhets/gitlab.git SHA256E'
         subject.send :parse_cmd
       end
 
@@ -97,7 +101,7 @@ describe GitlabShell do
 
   describe :exec do
     context 'git-upload-pack' do
-      before { ssh_cmd 'git-upload-pack gitlab-ci.git' }
+      let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
       after { subject.exec }
 
       it "should process the command" do
@@ -108,10 +112,6 @@ describe GitlabShell do
         subject.should_receive(:exec_cmd).with("git-upload-pack", File.join(tmp_repos_path, 'gitlab-ci.git'))
       end
 
-      it "should set the GL_ID environment variable" do
-        ENV.should_receive("[]=").with("GL_ID", key_id)
-      end
-
       it "should log the command execution" do
         message = "gitlab-shell: executing git command "
         message << "<git-upload-pack #{File.join(tmp_repos_path, 'gitlab-ci.git')}> "
@@ -126,7 +126,7 @@ describe GitlabShell do
     end
 
     context 'git-receive-pack' do
-      before { ssh_cmd 'git-receive-pack gitlab-ci.git' }
+      let(:ssh_cmd) { 'git-receive-pack gitlab-ci.git' }
       after { subject.exec }
 
       it "should process the command" do
@@ -146,7 +146,7 @@ describe GitlabShell do
     end
 
     context 'arbitrary command' do
-      before { ssh_cmd 'arbitrary command' }
+      let(:ssh_cmd) { 'arbitrary command' }
       after { subject.exec }
 
       it "should not process the command" do
@@ -163,8 +163,7 @@ describe GitlabShell do
       end
     end
 
-    context 'no command' do
-      before { ssh_cmd nil }
+    context 'no command' do      
       after { subject.exec }
 
       it "should call api.discover" do
@@ -173,8 +172,9 @@ describe GitlabShell do
     end
 
     context "failed connection" do
+      let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
+
       before {
-        ssh_cmd 'git-upload-pack gitlab-ci.git'
         api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError)
       }
       after { subject.exec }
@@ -189,9 +189,10 @@ describe GitlabShell do
     end
 
     describe 'git-annex' do
+      let(:ssh_cmd) { 'git-annex-shell commit /~/gitlab-ci.git SHA256' }
+
       before do
         GitlabConfig.any_instance.stub(git_annex_enabled?: true)
-        ssh_cmd 'git-annex-shell commit /~/gitlab-ci.git SHA256'
       end
 
       after { subject.exec }
@@ -203,7 +204,7 @@ describe GitlabShell do
   end
 
   describe :validate_access do
-    before { ssh_cmd 'git-upload-pack gitlab-ci.git' }
+    let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
     after { subject.exec }
 
     it "should call api.check_access" do
@@ -220,7 +221,7 @@ describe GitlabShell do
   end
 
   describe :exec_cmd do
-    let(:shell) { GitlabShell.new }
+    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
     before { Kernel.stub!(:exec) }
 
     it "uses Kernel::exec method" do
@@ -230,21 +231,17 @@ describe GitlabShell do
   end
 
   describe :api do
-    let(:shell) { GitlabShell.new }
+    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
     subject { shell.send :api }
 
     it { should be_a(GitlabNet) }
   end
 
   describe :escape_path do
-    let(:shell) { GitlabShell.new }
+    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
     before { File.stub(:absolute_path) { 'y' } }
     subject { -> { shell.send(:escape_path, 'z') } }
 
     it { should raise_error(GitlabShell::InvalidRepositoryPathError) }
   end
-
-  def ssh_cmd(cmd)
-    ENV['SSH_ORIGINAL_COMMAND'] = cmd
-  end
 end