Merge branch 'internal-api-unavailable' into 'master'

Show nice error message when internal API is unreachable.

Closes #27.

Error message reads "Failed to authorize your Git request: internal API unreachable".

See merge request !54

bin/check

@@ -8,13 +8,18 @@ require_relative '../lib/gitlab_net'
 #
 
 print "Check GitLab API access: "
-resp = GitlabNet.new.check
-if resp.code == "200"
-  print 'OK'
-else
-  abort "FAILED. code: #{resp.code}"
+begin
+  resp = GitlabNet.new.check
+  if resp.code == "200"
+    print 'OK'
+  else
+    abort "FAILED. code: #{resp.code}"
+  end
+rescue GitlabNet::ApiUnreachableError
+  abort "FAILED: Failed to connect to internal API"
 end
 
+
 puts "\nCheck directories and files: "
 
 config = GitlabConfig.new

lib/gitlab_access.rb

@@ -18,15 +18,20 @@ class GitlabAccess
   end
 
   def exec
-    status = api.check_access('git-receive-pack', @repo_name, @actor, @changes)
-    if status.allowed?
-      true
-    else
-      # reset GL_ID env since we stop git push here
-      ENV['GL_ID'] = nil
-      puts "GitLab: #{status.message}"
-      false
+    begin
+      status = api.check_access('git-receive-pack', @repo_name, @actor, @changes)
+
+      return true if status.allowed?
+
+      message = status.message
+    rescue GitlabNet::ApiUnreachableError
+      message = "Failed to authorize your Git request: internal API unreachable"
     end
+
+    # reset GL_ID env since we stop git push here
+    ENV['GL_ID'] = nil
+    puts "GitLab: #{message}"
+    false
   end
 
   protected

lib/gitlab_net.rb

@@ -7,6 +7,8 @@ require_relative 'gitlab_logger'
 require_relative 'gitlab_access'
 
 class GitlabNet
+  class ApiUnreachableError < StandardError; end
+
   def check_access(cmd, repo, actor, changes)
     project_name = repo.gsub("'", "")
     project_name = project_name.gsub(/\.git\Z/, "")
@@ -64,63 +66,65 @@ class GitlabNet
     "#{config.gitlab_url}/api/v3/internal"
   end
 
-  def http_client_for(url)
-    Net::HTTP.new(url.host, url.port).tap do |http|
-      if URI::HTTPS === url
-        http.use_ssl = true
-        http.cert_store = cert_store
-        http.verify_mode = OpenSSL::SSL::VERIFY_NONE if config.http_settings['self_signed_cert']
-      end
+  def http_client_for(uri)
+    http = Net::HTTP.new(uri.host, uri.port)
+
+    if uri.is_a?(URI::HTTPS)
+      http.use_ssl = true
+      http.cert_store = cert_store
+      http.verify_mode = OpenSSL::SSL::VERIFY_NONE if config.http_settings['self_signed_cert']
     end
+
+    http
   end
 
-  def http_request_for(url, method = :get)
+  def http_request_for(method, uri, params = {})
+    request_klass = method == :get ? Net::HTTP::Get : Net::HTTP::Post
+    request = request_klass.new(uri.request_uri)
+
     user = config.http_settings['user']
     password = config.http_settings['password']
+    request.basic_auth(user, password) if user && password
+
+    request.set_form_data(params.merge(secret_token: secret_token))
+
+    request
+  end
+
+  def request(method, url, params = {})
+    $logger.debug "Performing #{method.to_s.upcase} #{url}"
+
+    uri = URI.parse(url)
+    
+    http = http_client_for(uri)
+    request = http_request_for(method, uri, params)
 
-    if method == :get
-      Net::HTTP::Get.new(url.request_uri).tap { |r| r.basic_auth(user, password) if user && password }
+    begin
+      response = http.start { http.request(request) }
+    rescue 
+      raise ApiUnreachableError 
+    end
+
+    if response.code == "200"
+      $logger.debug "Received response #{response.code} => <#{response.body}>."
     else
-      Net::HTTP::Post.new(url.request_uri).tap { |r| r.basic_auth(user, password) if user && password }
+      $logger.error "API call <#{method.to_s.upcase} #{url}> failed: #{response.code} => <#{response.body}>."
     end
+
+    response
   end
 
   def get(url)
-    $logger.debug "Performing GET #{url}"
-
-    url = URI.parse(url)
-    http = http_client_for url
-    request = http_request_for url
-    request.set_form_data(secret_token: secret_token)
-
-    http.start { |http| http.request(request) }.tap do |resp|
-      if resp.code == "200"
-        $logger.debug { "Received response #{resp.code} => <#{resp.body}>." }
-      else
-        $logger.error { "API call <GET #{url}> failed: #{resp.code} => <#{resp.body}>." }
-      end
-    end
+    request(:get, url)
   end
 
   def post(url, params)
-    $logger.debug "Performing POST #{url}"
-
-    url = URI.parse(url)
-    http = http_client_for(url)
-    request = http_request_for(url, :post)
-    request.set_form_data(params.merge(secret_token: secret_token))
-
-    http.start { |http| http.request(request) }.tap do |resp|
-      if resp.code == "200"
-        $logger.debug { "Received response #{resp.code} => <#{resp.body}>." }
-      else
-        $logger.error { "API call <POST #{url}> failed: #{resp.code} => <#{resp.body}>." }
-      end
-    end
+    request(:post, url, params)
   end
 
   def cert_store
-    @cert_store ||= OpenSSL::X509::Store.new.tap do |store|
+    @cert_store ||= begin
+      store = OpenSSL::X509::Store.new
       store.set_default_paths
 
       if ca_file = config.http_settings['ca_file']
@@ -130,6 +134,8 @@ class GitlabNet
       if ca_path = config.http_settings['ca_path']
         store.add_path(ca_path)
       end
+
+      store
     end
   end
 

lib/gitlab_post_receive.rb

@@ -18,9 +18,13 @@ class GitlabPostReceive
 
     update_redis
 
-    if broadcast_message = GitlabNet.new.broadcast_message
-      puts
-      print_broadcast_message(broadcast_message["message"])
+    begin
+      broadcast_message = GitlabNet.new.broadcast_message
+      if broadcast_message
+        puts
+        print_broadcast_message(broadcast_message["message"])
+      end
+    rescue GitlabNet::ApiUnreachableError
     end
   end
 

lib/gitlab_shell.rb

@@ -21,12 +21,13 @@ class GitlabShell
       if git_cmds.include?(@git_cmd)
         ENV['GL_ID'] = @key_id
 
-        if validate_access
+        access = api.check_access(@git_cmd, @repo_name, @key_id, '_any')
+        if access.allowed?
           process_cmd
         else
           message = "gitlab-shell: Access denied for git command <#{@origin_cmd}> by #{log_username}."
           $logger.warn message
-          $stderr.puts "Access denied."
+          puts access.message
         end
       else
         raise DisallowedCommandError
@@ -34,10 +35,13 @@ class GitlabShell
     else
       puts "Welcome to GitLab, #{username}!"
     end
+  rescue GitlabNet::ApiUnreachableError => ex
+    $logger.warn "gitlab-shell: Failed to connect to internal API"
+    puts "Failed to authorize your Git request: internal API unreachable"
   rescue DisallowedCommandError => ex
     message = "gitlab-shell: Attempt to execute disallowed command <#{@origin_cmd}> by #{log_username}."
     $logger.warn message
-    puts 'Not allowed command'
+    puts 'Disallowed command'
   end
 
   protected
@@ -59,10 +63,6 @@ class GitlabShell
     exec_cmd(@git_cmd, repo_full_path)
   end
 
-  def validate_access
-    api.check_access(@git_cmd, @repo_name, @key_id, '_any').allowed?
-  end
-
   # 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)
@@ -73,11 +73,12 @@ class GitlabShell
   end
 
   def user
-    # Can't use "@user ||=" because that will keep hitting the API when @user is really nil!
-    if instance_variable_defined?('@user')
-      @user
-    else
+    return @user if defined?(@user)
+
+    begin
       @user = api.discover(@key_id)
+    rescue GitlabNet::ApiUnreachableError
+      @user = nil
     end
   end
 

spec/gitlab_access_spec.rb

@@ -5,15 +5,56 @@ describe GitlabAccess do
   let(:repository_path) { "/home/git/repositories" }
   let(:repo_name)   { 'dzaporozhets/gitlab-ci' }
   let(:repo_path)  { File.join(repository_path, repo_name) + ".git" }
-  let(:gitlab_access) { GitlabAccess.new(repo_path, 'key-123', 'wow') }
+  let(:api) do
+    double(GitlabNet).tap do |api|
+      api.stub(check_access: GitAccessStatus.new(true))
+    end
+  end
+  subject do
+    GitlabAccess.new(repo_path, 'key-123', 'wow').tap do |access|
+      access.stub(exec_cmd: :exec_called)
+      access.stub(api: api)
+    end
+  end
 
   before do
     GitlabConfig.any_instance.stub(repos_path: repository_path)
   end
 
   describe :initialize do
-    it { gitlab_access.repo_name.should == repo_name }
-    it { gitlab_access.repo_path.should == repo_path }
-    it { gitlab_access.changes.should == ['wow'] }
+    it { subject.repo_name.should == repo_name }
+    it { subject.repo_path.should == repo_path }
+    it { subject.changes.should == ['wow'] }
+  end
+
+  describe "#exec" do
+    context "access is granted" do
+
+      it "returns true" do
+        expect(subject.exec).to be_true
+      end
+    end
+
+    context "access is denied" do
+
+      before do
+        api.stub(check_access: GitAccessStatus.new(false))
+      end
+
+      it "returns false" do
+        expect(subject.exec).to be_false
+      end
+    end
+
+    context "API connection fails" do
+
+      before do
+        api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError)
+      end
+
+      it "returns false" do
+        expect(subject.exec).to be_false
+      end
+    end
   end
 end

spec/gitlab_net_spec.rb

@@ -26,6 +26,11 @@ describe GitlabNet, vcr: true do
         gitlab_net.check
       end
     end
+
+    it "raises an exception if the connection fails" do
+      Net::HTTP.any_instance.stub(:request).and_raise(StandardError)
+      expect { gitlab_net.check }.to raise_error(GitlabNet::ApiUnreachableError)
+    end
   end
 
   describe :discover do
@@ -42,6 +47,13 @@ describe GitlabNet, vcr: true do
         gitlab_net.discover('key-126')
       end
     end
+
+    it "raises an exception if the connection fails" do
+      VCR.use_cassette("discover-ok") do
+        Net::HTTP.any_instance.stub(:request).and_raise(StandardError)
+        expect { gitlab_net.discover('key-126') }.to raise_error(GitlabNet::ApiUnreachableError)
+      end
+    end
   end
 
   describe :broadcast_message do
@@ -110,6 +122,13 @@ describe GitlabNet, vcr: true do
         end
       end
     end
+
+    it "raises an exception if the connection fails" do
+      Net::HTTP.any_instance.stub(:request).and_raise(StandardError)
+      expect { 
+        gitlab_net.check_access('git-upload-pack', 'gitlab/gitlabhq.git', 'user-1', changes)
+      }.to raise_error(GitlabNet::ApiUnreachableError)
+    end
   end
 
   describe :host do
@@ -139,12 +158,13 @@ describe GitlabNet, vcr: true do
     let(:user) { 'user' }
     let(:password) { 'password' }
     let(:url) { URI 'http://localhost/' }
-    subject { gitlab_net.send :http_request_for, url }
+    subject { gitlab_net.send :http_request_for, :get, url }
 
     before do
       gitlab_net.send(:config).http_settings.stub(:[]).with('user') { user }
       gitlab_net.send(:config).http_settings.stub(:[]).with('password') { password }
       get.should_receive(:basic_auth).with(user, password).once
+      get.should_receive(:set_form_data).with(hash_including(secret_token: 'a123')).once
     end
 
     it { should_not be_nil }

spec/gitlab_shell_spec.rb

@@ -135,6 +135,27 @@ describe GitlabShell do
         api.should_receive(:discover).with(key_id)
       end
     end
+
+    context "failed connection" do
+      before { 
+        ssh_cmd 'git-upload-pack gitlab-ci.git'
+        api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError)
+      }
+      after { subject.exec }
+
+      it "should not process the command" do
+        subject.should_not_receive(:process_cmd)
+      end
+
+      it "should not execute the command" do
+        subject.should_not_receive(:exec_cmd)
+      end
+
+      it "should log the failed connection" do
+        message = "gitlab-shell: Failed to connect to internal API"
+        $logger.should_receive(:warn).with(message)
+      end
+    end
   end
 
   describe :validate_access do