Improve encapsulation of GitlabShell

- Remove public attr_access and attr_reader (no public access required)
- Replace protected with private as GitlabShell is not inherited anywhere
- Create private attr_access and attr_reader that are more accurate
- Remove @ from variables where they're referenced but not set to bring further clarity

2 files changed, 29 insertions, 34 deletions

diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index e30e5e1..c1f3292 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -21,9 +21,6 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   API_COMMANDS = %w(2fa_recovery_codes).freeze
   GL_PROTOCOL = 'ssh'.freeze
 
-  attr_accessor :key_id, :gl_repository, :repo_name, :command, :git_access
-  attr_reader :repo_path
-
   def initialize(key_id)
     @key_id = key_id
     @config = GitlabConfig.new
@@ -53,12 +50,10 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     false
   rescue AccessDeniedError => ex
     $logger.warn('Access denied', command: origin_cmd, user: log_username)
-
     $stderr.puts "GitLab: #{ex.message}"
     false
   rescue DisallowedCommandError
     $logger.warn('Denied disallowed command', command: origin_cmd, user: log_username)
-
     $stderr.puts "GitLab: Disallowed command"
     false
   rescue InvalidRepositoryPathError
@@ -66,24 +61,27 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     false
   end
 
-  protected
+  private
+
+  attr_accessor :repo_name, :command, :git_access
+  attr_reader :config, :key_id, :repo_path
 
   def parse_cmd(args)
     # Handle Git for Windows 2.14 using "git upload-pack" instead of git-upload-pack
     if args.length == 3 && args.first == 'git'
       @command = "git-#{args[1]}"
-      args = [@command, args.last]
+      args = [command, args.last]
     else
       @command = args.first
     end
 
-    @git_access = @command
+    @git_access = command
 
-    return args if API_COMMANDS.include?(@command)
+    return args if API_COMMANDS.include?(command)
 
-    raise DisallowedCommandError unless GIT_COMMANDS.include?(@command)
+    raise DisallowedCommandError unless GIT_COMMANDS.include?(command)
 
-    case @command
+    case command
     when 'git-lfs-authenticate'
       raise DisallowedCommandError unless args.count >= 2
       @repo_name = args[1]
@@ -104,7 +102,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   end
 
   def verify_access
-    status = api.check_access(@git_access, nil, @repo_name, @key_id, '_any', GL_PROTOCOL)
+    status = api.check_access(git_access, nil, repo_name, key_id, '_any', GL_PROTOCOL)
 
     raise AccessDeniedError, status.message unless status.allowed?
 
@@ -115,9 +113,9 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   end
 
   def process_cmd(args)
-    return send("api_#{@command}") if API_COMMANDS.include?(@command)
+    return send("api_#{command}") if API_COMMANDS.include?(command)
 
-    if @command == 'git-lfs-authenticate'
+    if command == 'git-lfs-authenticate'
       GitlabMetrics.measure('lfs-authenticate') do
         $logger.info('Processing LFS authentication', user: log_username)
         lfs_authenticate
@@ -125,7 +123,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       return
     end
 
-    executable = @command
+    executable = command
     args = [repo_path]
 
     if GITALY_MIGRATED_COMMANDS.key?(executable) && @gitaly
@@ -138,7 +136,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       gitaly_request = {
         'repository' => @gitaly['repository'],
         'gl_repository' => @gl_repository,
-        'gl_id' => @key_id,
+        'gl_id' => key_id,
         'gl_username' => @username
       }
 
@@ -164,7 +162,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       'PATH' => ENV['PATH'],
       'LD_LIBRARY_PATH' => ENV['LD_LIBRARY_PATH'],
       'LANG' => ENV['LANG'],
-      'GL_ID' => @key_id,
+      'GL_ID' => key_id,
       'GL_PROTOCOL' => GL_PROTOCOL,
       'GL_REPOSITORY' => @gl_repository,
       'GL_USERNAME' => @username
@@ -175,9 +173,9 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
     if git_trace_available?
       env.merge!(
-        'GIT_TRACE' => @config.git_trace_log_file,
-        'GIT_TRACE_PACKET' => @config.git_trace_log_file,
-        'GIT_TRACE_PERFORMANCE' => @config.git_trace_log_file
+        'GIT_TRACE' => config.git_trace_log_file,
+        'GIT_TRACE_PACKET' => config.git_trace_log_file,
+        'GIT_TRACE_PERFORMANCE' => config.git_trace_log_file
       )
     end
 
@@ -193,7 +191,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     return @user if defined?(@user)
 
     begin
-      @user = api.discover(@key_id)
+      @user = api.discover(key_id)
     rescue GitlabNet::ApiUnreachableError
       @user = nil
     end
@@ -211,32 +209,29 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
   # User identifier to be used in log messages.
   def log_username
-    @config.audit_usernames ? username : "user with key #{@key_id}"
+    config.audit_usernames ? username : "user with key #{key_id}"
   end
 
   def lfs_authenticate
-    lfs_access = api.lfs_authenticate(@key_id, @repo_name)
-
+    lfs_access = api.lfs_authenticate(key_id, repo_name)
     return unless lfs_access
 
     puts lfs_access.authentication_payload
   end
 
-  private
-
   def git_trace_available?
-    return false unless @config.git_trace_log_file
+    return false unless config.git_trace_log_file
 
-    if Pathname(@config.git_trace_log_file).relative?
-      $logger.warn('git trace log path must be absolute, ignoring', git_trace_log_file: @config.git_trace_log_file)
+    if Pathname(config.git_trace_log_file).relative?
+      $logger.warn('git trace log path must be absolute, ignoring', git_trace_log_file: config.git_trace_log_file)
       return false
     end
 
     begin
-      File.open(@config.git_trace_log_file, 'a') { nil }
+      File.open(config.git_trace_log_file, 'a') { nil }
       return true
     rescue => ex
-      $logger.warn('Failed to open git trace log file', git_trace_log_file: @config.git_trace_log_file, error: ex.to_s)
+      $logger.warn('Failed to open git trace log file', git_trace_log_file: config.git_trace_log_file, error: ex.to_s)
       return false
     end
   end
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index af84b29..6d07b1c 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -108,11 +108,11 @@ describe GitlabShell do
           let(:ssh_args) { %w(2fa_recovery_codes) }
 
           it 'sets the correct command' do
-            expect(subject.command).to eq('2fa_recovery_codes')
+            expect(subject.send(:command)).to eq('2fa_recovery_codes') # FIXME: don't access private instance variables
           end
 
           it 'does not set repo name' do
-            expect(subject.repo_name).to be_nil
+            expect(subject.send(:repo_name)).to be_nil # FIXME: don't access private instance variables
           end
         end
       end
@@ -452,7 +452,7 @@ describe GitlabShell do
     let(:exec_options) { { unsetenv_others: true, chdir: ROOT_PATH } }
     before do
       Kernel.stub(:exec)
-      shell.gl_repository = gl_repository
+      shell.instance_variable_set(:@gl_repository, gl_repository)
       shell.instance_variable_set(:@username, gl_username)
     end