Merge branch 'sh-bump-ruby-version' into 'master'

Upgrade to Ruby 2.5.3

See merge request gitlab-org/gitlab-shell!257

.gitlab-ci.yml

-image: "ruby:2.3"
+image: "ruby:2.5"
 
 before_script:
   - export PATH=~/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin
@@ -30,18 +30,18 @@ rubocop:
   except:
     - tags
 
-#ruby 2.2
-rspec:ruby2.2:
-  image: ruby:2.2
+#ruby 2.4
+rspec:ruby2.4:
+  image: ruby:2.4
   <<: *rspec_definition
   tags:
     - ruby
   except:
     - tags
 
-#ruby 2.1
-rspec:ruby2.1:
-  image: ruby:2.1
+#ruby 2.3
+rspec:ruby2.3:
+  image: ruby:2.3
   <<: *rspec_definition
   tags:
     - ruby
@@ -77,7 +77,7 @@ go:1.10:
 
 go:1.11:
   <<: *go_definition
-  image: golang:1.10
+  image: golang:1.11
 
 codequality:
   image: docker:stable

.rubocop.yml

@@ -6,6 +6,7 @@ AllCops:
     - 'tmp/**/*'
     - 'bin/**/*'
     - 'hooks/**/*'
+    - 'support/**/*'
     - 'Guardfile'
 
 Layout/DotPosition:
@@ -35,6 +36,9 @@ Metrics/CyclomaticComplexity:
 Metrics/PerceivedComplexity:
   Enabled: false
 
+Naming/AccessorMethodName:
+  Enabled: false
+
 Style/Documentation:
   Enabled: false
 
@@ -47,9 +51,6 @@ Style/StringLiterals:
 Style/GlobalVars:
   Enabled: false
 
-Style/AccessorMethodName:
-  Enabled: false
-
 Style/GuardClause:
   Enabled: false
 

.ruby-version

-2.3.7
+2.5.3

Gemfile

 source 'https://rubygems.org'
 
+group :development do
+  gem 'pry-byebug', '~> 3.6', require: false
+end
+
 group :development, :test do
   gem 'listen', '~> 0.5.0'
   gem 'rspec', '~> 3.8.0'
   gem 'rspec-parameterized', '~> 0.4.0'
-  gem 'rubocop', '0.49.1', require: false
-  gem 'simplecov', '~> 0.9.0', require: false
+  gem 'rubocop', '0.61', require: false
+  gem 'simplecov', '~> 0.16.1', require: false
   gem 'vcr', '~> 4.0.0'
   gem 'webmock', '~> 3.4.0'
 end

Gemfile.lock

@@ -8,26 +8,27 @@ GEM
     addressable (2.5.2)
       public_suffix (>= 2.0.2, < 4.0)
     ast (2.4.0)
-    binding_of_caller (0.8.0)
-      debug_inspector (>= 0.0.1)
+    binding_ninja (0.2.2)
+    byebug (10.0.2)
     coderay (1.1.2)
     concord (0.1.5)
       adamantium (~> 0.2.0)
       equalizer (~> 0.0.9)
     crack (0.4.3)
       safe_yaml (~> 1.0.0)
-    debug_inspector (0.0.3)
     diff-lcs (1.3)
-    docile (1.1.5)
+    docile (1.3.1)
     equalizer (0.0.11)
     hashdiff (0.3.7)
     ice_nine (0.11.2)
+    jaro_winkler (1.5.1)
+    json (2.1.0)
     listen (0.5.3)
     memoizable (0.4.2)
       thread_safe (~> 0.3, >= 0.3.1)
-    multi_json (1.13.1)
+    method_source (0.9.2)
     parallel (1.12.1)
-    parser (2.5.1.2)
+    parser (2.5.3.0)
       ast (~> 2.4.0)
     powerpack (0.1.2)
     proc_to_ast (0.1.0)
@@ -35,46 +36,51 @@ GEM
       parser
       unparser
     procto (0.0.3)
+    pry (0.12.2)
+      coderay (~> 1.1.0)
+      method_source (~> 0.9.0)
+    pry-byebug (3.6.0)
+      byebug (~> 10.0)
+      pry (~> 0.10)
     public_suffix (3.0.3)
-    rainbow (2.2.2)
-      rake
-    rake (12.3.1)
+    rainbow (3.0.0)
     rspec (3.8.0)
       rspec-core (~> 3.8.0)
       rspec-expectations (~> 3.8.0)
       rspec-mocks (~> 3.8.0)
     rspec-core (3.8.0)
       rspec-support (~> 3.8.0)
-    rspec-expectations (3.8.1)
+    rspec-expectations (3.8.2)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.8.0)
     rspec-mocks (3.8.0)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.8.0)
-    rspec-parameterized (0.4.0)
-      binding_of_caller
+    rspec-parameterized (0.4.1)
+      binding_ninja (>= 0.2.1)
       parser
       proc_to_ast
       rspec (>= 2.13, < 4)
       unparser
     rspec-support (3.8.0)
-    rubocop (0.49.1)
+    rubocop (0.61.0)
+      jaro_winkler (~> 1.5.1)
       parallel (~> 1.10)
-      parser (>= 2.3.3.1, < 3.0)
+      parser (>= 2.5, != 2.5.1.1)
       powerpack (~> 0.1)
-      rainbow (>= 1.99.1, < 3.0)
+      rainbow (>= 2.2.2, < 4.0)
       ruby-progressbar (~> 1.7)
-      unicode-display_width (~> 1.0, >= 1.0.1)
-    ruby-progressbar (1.9.0)
+      unicode-display_width (~> 1.4.0)
+    ruby-progressbar (1.10.0)
     safe_yaml (1.0.4)
-    simplecov (0.9.2)
-      docile (~> 1.1.0)
-      multi_json (~> 1.0)
-      simplecov-html (~> 0.9.0)
-    simplecov-html (0.9.0)
+    simplecov (0.16.1)
+      docile (~> 1.1)
+      json (>= 1.8, < 3)
+      simplecov-html (~> 0.10.0)
+    simplecov-html (0.10.2)
     thread_safe (0.3.6)
     unicode-display_width (1.4.0)
-    unparser (0.2.8)
+    unparser (0.4.2)
       abstract_type (~> 0.0.7)
       adamantium (~> 0.2.0)
       concord (~> 0.1.5)
@@ -93,12 +99,13 @@ PLATFORMS
 
 DEPENDENCIES
   listen (~> 0.5.0)
+  pry-byebug (~> 3.6)
   rspec (~> 3.8.0)
   rspec-parameterized (~> 0.4.0)
-  rubocop (= 0.49.1)
-  simplecov (~> 0.9.0)
+  rubocop (= 0.61)
+  simplecov (~> 0.16.1)
   vcr (~> 4.0.0)
   webmock (~> 3.4.0)
 
 BUNDLED WITH
-   1.16.3
+   1.17.1

lib/action/custom.rb

@@ -96,12 +96,13 @@ module Action
 
     def print_flush(str)
       return false unless str
+
       $stdout.print(Base64.decode64(str))
       $stdout.flush
     end
 
     def inform_client(str)
-      $stderr.puts(format_gitlab_output(str))
+      warn(format_gitlab_output(str))
     end
 
     def format_gitlab_output(str)

lib/gitlab_access.rb

@@ -31,10 +31,10 @@ class GitlabAccess
 
     true
   rescue GitlabNet::ApiUnreachableError
-    $stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable"
+    warn "GitLab: Failed to authorize your Git request: internal API unreachable"
     false
   rescue AccessDeniedError => ex
-    $stderr.puts "GitLab: #{ex.message}"
+    warn "GitLab: #{ex.message}"
     false
   end
 

lib/gitlab_keys.rb

 require 'timeout'
+require 'open3'
 
 require_relative 'gitlab_config'
 require_relative 'gitlab_logger'
@@ -14,7 +15,7 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
   end
 
   def self.command_key(key_id)
-    unless /\A[a-z0-9-]+\z/ =~ key_id
+    unless /\A[a-z0-9-]+\z/ =~ key_id # rubocop:disable Performance/RegexpMatch
       raise KeyError, "Invalid key_id: #{key_id.inspect}"
     end
 
@@ -107,7 +108,9 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
     open_auth_file('r') do |f|
       f.each_line do |line|
         matchd = line.match(/key-(\d+)/)
+
         next unless matchd
+
         puts matchd[1]
       end
     end
@@ -138,6 +141,7 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
       open_auth_file('r+') do |f|
         while line = f.gets # rubocop:disable Lint/AssignmentInCondition
           next unless line.start_with?("command=\"#{self.class.command_key(@key_id)}\"")
+
           f.seek(-line.length, IO::SEEK_CUR)
           # Overwrite the line with #'s. Because the 'line' variable contains
           # a terminating '\n', we write line.length - 1 '#' characters.
@@ -155,20 +159,24 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
 
   def check_permissions
     open_auth_file(File::RDWR | File::CREAT) { true }
-  rescue => ex
+  rescue StandardError => ex
     puts "error: could not open #{auth_file}: #{ex}"
-    if File.exist?(auth_file)
-      system('ls', '-l', auth_file)
-    else
-      # Maybe the parent directory is not writable?
-      system('ls', '-ld', File.dirname(auth_file))
-    end
+
+    cmd = if File.exist?(auth_file)
+            %W{ls -l #{auth_file}}
+          else
+            # Maybe the parent directory is not writable?
+            %W{ls -ld #{File.dirname(auth_file)}}
+          end
+
+    output, = Open3.capture2e(cmd.join(' '))
+    puts output
     false
   end
 
   def lock(timeout = 10)
     File.open(lock_file, "w+") do |f|
-      begin
+      begin # rubocop:disable Style/RedundantBegin
         f.flock File::LOCK_EX
         Timeout.timeout(timeout) { yield }
       ensure
@@ -182,7 +190,7 @@ class GitlabKeys # rubocop:disable Metrics/ClassLength
   end
 
   def open_auth_file(mode)
-    open(auth_file, mode, 0o600) do |file|
+    File.open(auth_file, mode, 0o600) do |file|
       file.chmod(0o600)
       yield file
     end

lib/gitlab_lfs_authentication.rb

@@ -13,7 +13,7 @@ class GitlabLfsAuthentication
   def self.build_from_json(json)
     values = JSON.parse(json)
     new(values['username'], values['lfs_token'], values['repository_http_path'])
-  rescue
+  rescue StandardError
     nil
   end
 

lib/gitlab_logger.rb

@@ -7,15 +7,14 @@ require_relative 'gitlab_config'
 def convert_log_level(log_level)
   Logger.const_get(log_level.upcase)
 rescue NameError
-  $stderr.puts "WARNING: Unrecognized log level #{log_level.inspect}."
-  $stderr.puts "WARNING: Falling back to INFO."
+  warn "WARNING: Unrecognized log level #{log_level.inspect}. Falling back to INFO."
   Logger::INFO
 end
 
 class GitlabLogger
   # Emulate the quoting logic of logrus
   # https://github.com/sirupsen/logrus/blob/v1.0.5/text_formatter.go#L143-L156
-  SHOULD_QUOTE = /[^a-zA-Z0-9\-._\/@^+]/
+  SHOULD_QUOTE = /[^a-zA-Z0-9\-._\/@^+]/.freeze
 
   LEVELS = {
     Logger::INFO => 'info'.freeze,

lib/gitlab_net.rb

+# frozen_string_literal: true
+
 require 'net/http'
 require 'openssl'
 require 'json'
@@ -11,7 +13,7 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   include HTTPHelper
 
   CHECK_TIMEOUT = 5
-  API_INACCESSIBLE_MESSAGE = 'API is not accessible'.freeze
+  API_INACCESSIBLE_MESSAGE = 'API is not accessible'
 
   def check_access(cmd, gl_repository, repo, who, changes, protocol, env: {})
     changes = changes.join("\n") unless changes.is_a?(String)
@@ -76,8 +78,8 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   def merge_request_urls(gl_repository, repo_path, changes)
     changes = changes.join("\n") unless changes.is_a?(String)
     changes = changes.encode('UTF-8', 'ASCII', invalid: :replace, replace: '')
-    url = "#{internal_api_endpoint}/merge_request_urls?project=#{URI.escape(repo_path)}&changes=#{URI.escape(changes)}"
-    url += "&gl_repository=#{URI.escape(gl_repository)}" if gl_repository
+    url = "#{internal_api_endpoint}/merge_request_urls?project=#{uri_escape(repo_path)}&changes=#{uri_escape(changes)}"
+    url += "&gl_repository=#{uri_escape(gl_repository)}" if gl_repository
     resp = get(url)
 
     if resp.code == '200'
@@ -85,7 +87,7 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     else
       []
     end
-  rescue
+  rescue StandardError
     []
   end
 
@@ -94,9 +96,9 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   end
 
   def authorized_key(key)
-    resp = get("#{internal_api_endpoint}/authorized_keys?key=#{URI.escape(key, '+/=')}")
+    resp = get("#{internal_api_endpoint}/authorized_keys?key=#{URI.escape(key, '+/=')}") # rubocop:disable Lint/UriEscapeUnescape
     JSON.parse(resp.body) if resp.code == "200"
-  rescue
+  rescue StandardError
     nil
   end
 
@@ -106,7 +108,7 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     resp = post("#{internal_api_endpoint}/two_factor_recovery_codes", id_sym => id)
 
     JSON.parse(resp.body) if resp.code == '200'
-  rescue
+  rescue StandardError
     {}
   end
 
@@ -115,7 +117,7 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     resp = post("#{internal_api_endpoint}/notify_post_receive", params)
 
     resp.code == '200'
-  rescue
+  rescue StandardError
     false
   end
 
@@ -143,11 +145,15 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   def self.parse_who(who)
     if who.start_with?("key-")
       value = who.gsub("key-", "")
+
       raise ArgumentError, "who='#{who}' is invalid!" unless value =~ /\A[0-9]+\z/
+
       [:key_id, 'key_id', value]
     elsif who.start_with?("user-")
       value = who.gsub("user-", "")
+
       raise ArgumentError, "who='#{who}' is invalid!" unless value =~ /\A[0-9]+\z/
+
       [:user_id, 'user_id', value]
     elsif who.start_with?("username-")
       [:username, 'username', who.gsub("username-", "")]
@@ -161,4 +167,8 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   def sanitize_path(repo)
     repo.delete("'")
   end
+
+  def uri_escape(str)
+    URI.escape(str) # rubocop:disable Lint/UriEscapeUnescape
+  end
 end

lib/gitlab_post_receive.rb

@@ -25,6 +25,7 @@ class GitlabPostReceive
     end
 
     return false unless response
+
     print_broadcast_message(response['broadcast_message']) if response['broadcast_message']
     print_merge_request_links(response['merge_request_urls']) if response['merge_request_urls']
     puts response['redirected_message'] if response['redirected_message']
@@ -43,6 +44,7 @@ class GitlabPostReceive
 
   def print_merge_request_links(merge_request_urls)
     return if merge_request_urls.empty?
+
     puts
     merge_request_urls.each { |mr| print_merge_request_link(mr) }
   end

lib/gitlab_shell.rb

@@ -81,23 +81,23 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
     true
   rescue GitlabNet::ApiUnreachableError
-    $stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable"
+    warn "GitLab: Failed to authorize your Git request: internal API unreachable"
     false
   rescue AccessDeniedError => ex
     $logger.warn('Access denied', command: origin_cmd, user: log_username)
-    $stderr.puts "GitLab: #{ex.message}"
+    warn "GitLab: #{ex.message}"
     false
   rescue DisallowedCommandError
     $logger.warn('Denied disallowed command', command: origin_cmd, user: log_username)
-    $stderr.puts "GitLab: Disallowed command"
+    warn "GitLab: Disallowed command"
     false
   rescue InvalidRepositoryPathError
-    $stderr.puts "GitLab: Invalid repository path"
+    warn "GitLab: Invalid repository path"
     false
   rescue Action::Custom::BaseError => ex
     $logger.warn('Custom action error', exception: ex.class, message: ex.message,
                                         command: origin_cmd, user: log_username)
-    $stderr.puts ex.message
+    warn ex.message
     false
   end
 
@@ -121,6 +121,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     case @command
     when GIT_LFS_AUTHENTICATE_COMMAND
       raise DisallowedCommandError unless args.count >= 2
+
       @repo_name = args[1]
       case args[2]
       when 'download'
@@ -132,6 +133,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       end
     else
       raise DisallowedCommandError unless args.count == 2
+
       @repo_name = args.last
     end
 
@@ -201,7 +203,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     begin
       if defined?(@who)
         @user = api.discover(@who)
-        @gl_id = "user-#{@user['id']}" if @user && @user.key?('id')
+        @gl_id = "user-#{@user['id']}" if @user&.key?('id')
       else
         @user = api.discover(@gl_id)
       end

lib/http_helper.rb

@@ -74,7 +74,7 @@ module HTTPHelper
     begin
       start_time = Time.new
       response = http.start { http.request(request) }
-    rescue => e
+    rescue StandardError => e
       $logger.warn('Failed to connect', method: method.to_s.upcase, url: url, error: e)
       raise GitlabNet::ApiUnreachableError
     ensure

lib/httpunix.rb

@@ -12,13 +12,14 @@ module URI
     def hostname
       # decode %XX from path to file
       v = host
-      URI.decode(v)
+      URI.decode(v) # rubocop:disable Lint/UriEscapeUnescape
     end
 
     # port is not allowed in URI
     DEFAULT_PORT = nil
-    def set_port(v)
-      return v unless v
+    def set_port(value)
+      return value unless value
+
       raise InvalidURIError, "http+unix:// cannot contain port"
     end
   end

spec/gitlab_keys_spec.rb

@@ -5,6 +5,8 @@ require 'stringio'
 describe GitlabKeys do
   before do
     $logger = double('logger').as_null_object
+    # The default 'auth_file' value from config.yml.example is '/home/git/.ssh/authorized_keys'
+    allow(GitlabConfig).to receive_message_chain(:new, :auth_file).and_return('/home/git/.ssh/authorized_keys')
   end
 
   describe '.command' do
@@ -148,7 +150,9 @@ describe GitlabKeys do
 
     context "without file writing" do
       before do
-        expect(gitlab_keys).to receive(:open).and_yield(double(:file, puts: nil, chmod: nil))
+        file = double(:file, puts: nil, chmod: nil, flock: nil)
+        expect(File).to receive(:open).with(tmp_authorized_keys_path + '.lock', 'w+').and_yield(file)
+        expect(File).to receive(:open).with(tmp_authorized_keys_path, "a", 0o600).and_yield(file)
       end
 
       it "should log an add-key event" do
@@ -189,7 +193,8 @@ describe GitlabKeys do
 
     context "without file writing" do
       before do
-        allow(gitlab_keys).to receive(:open)
+        allow(File).to receive(:open).with("#{ROOT_PATH}/config.yml", 'r:bom|utf-8').and_call_original
+        allow(File).to receive(:open).with('/home/git/.ssh/authorized_keys', 'r+', 384)
         allow(gitlab_keys).to receive(:lock).and_yield
       end
 
@@ -225,7 +230,8 @@ describe GitlabKeys do
     let(:gitlab_keys) { build_gitlab_keys('clear') }
 
     it "should return true" do
-      allow(gitlab_keys).to receive(:open)
+      allow(File).to receive(:open).with("#{ROOT_PATH}/config.yml", 'r:bom|utf-8').and_call_original
+      allow(File).to receive(:open).with('/home/git/.ssh/authorized_keys', 'w', 384)
       expect(gitlab_keys.send(:clear)).to be_truthy
     end
   end
@@ -240,7 +246,7 @@ describe GitlabKeys do
 
     it 'returns false if opening raises an exception' do
       expect(gitlab_keys).to receive(:open_auth_file).and_raise("imaginary error")
-      expect(gitlab_keys.exec).to eq(false)
+      expect { expect(gitlab_keys.exec).to eq(false) }.to output(/imaginary error/).to_stdout
     end
 
     it 'creates the keys file if it does not exist' do

spec/gitlab_logger_spec.rb

@@ -3,11 +3,13 @@ require_relative '../lib/gitlab_logger'
 require 'securerandom'
 
 describe :convert_log_level do
-  subject { convert_log_level :extreme }
+  it "returns desired Logger::<type>" do
+    expect(convert_log_level(:debug)).to eq(Logger::DEBUG)
+  end
 
   it "converts invalid log level to Logger::INFO" do
-    expect($stderr).to receive(:puts).at_least(:once)
-    is_expected.to eq(Logger::INFO)
+    expect { convert_log_level(:extreme) }.to output(/WARNING: Unrecognized log level :extreme. Falling back to INFO./).to_stderr
+    expect(convert_log_level(:extreme)).to eq(Logger::INFO)
   end
 end
 

spec/gitlab_net_spec.rb

@@ -67,7 +67,7 @@ describe GitlabNet, vcr: true do
 
   describe '#lfs_authenticate' do
     context 'lfs authentication succeeded' do
-      let(:repository_http_path) { URI.join(internal_api_endpoint.sub('api/v4', ''), project).to_s }
+      let(:repository_http_path) { URI.join(internal_api_endpoint.sub('/api/v4/internal', ''), project).to_s }
 
       context 'for download operation' do
         it 'should return the correct data' do

spec/gitlab_shell_gitlab_shell_spec.rb

 require_relative 'spec_helper'
+require 'open3'
 
 describe 'bin/gitlab-shell' do
   def original_root_path
@@ -131,12 +132,13 @@ describe 'bin/gitlab-shell' do
 
   def run!(args)
     cmd = [
+      'SSH_CONNECTION=fake',
       gitlab_shell_path,
       args
-    ].flatten.compact
+    ].flatten.join(' ')
 
-    output = IO.popen({'SSH_CONNECTION' => 'fake'}, cmd, &:read)
+    stdout, _stderr, status = Open3.capture3(cmd)
 
-    [output, $?]
+    [stdout, status]
   end
 end

spec/vcr_cassettes/custom-action-ok-not-json.yml

@@ -46,6 +46,6 @@ http_interactions:
       - '1.436040'
     body:
       encoding: UTF-8
-      string: '""'
+      string: '{'
     http_version:
   recorded_at: Fri, 20 Jul 2018 06:18:58 GMT