Merge branch '25095-remove-gitlab-shell-indirection-for-authorized-keys' into 'master'

Remove gitlab-shell indirection for authorized_keys changes

See merge request gitlab-org/gitlab!26913

app/models/key.rb

@@ -30,10 +30,10 @@ class Key < ApplicationRecord
 
   delegate :name, :email, to: :user, prefix: true
 
-  after_commit :add_to_shell, on: :create
+  after_commit :add_to_authorized_keys, on: :create
   after_create :post_create_hook
   after_create :refresh_user_cache
-  after_commit :remove_from_shell, on: :destroy
+  after_commit :remove_from_authorized_keys, on: :destroy
   after_destroy :post_destroy_hook
   after_destroy :refresh_user_cache
 
@@ -79,12 +79,10 @@ class Key < ApplicationRecord
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-  def add_to_shell
-    GitlabShellWorker.perform_async(
-      :add_key,
-      shell_id,
-      key
-    )
+  def add_to_authorized_keys
+    return unless Gitlab::CurrentSettings.authorized_keys_enabled?
+
+    AuthorizedKeysWorker.perform_async(:add_key, shell_id, key)
   end
 
   # rubocop: disable CodeReuse/ServiceClass
@@ -93,11 +91,10 @@ class Key < ApplicationRecord
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-  def remove_from_shell
-    GitlabShellWorker.perform_async(
-      :remove_key,
-      shell_id
-    )
+  def remove_from_authorized_keys
+    return unless Gitlab::CurrentSettings.authorized_keys_enabled?
+
+    AuthorizedKeysWorker.perform_async(:remove_key, shell_id)
   end
 
   # rubocop: disable CodeReuse/ServiceClass

app/workers/all_queues.yml

@@ -843,6 +843,13 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
+- :name: authorized_keys
+  :feature_category: :source_code_management
+  :has_external_dependencies: 
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 2
+  :idempotent: true
 - :name: authorized_projects
   :feature_category: :authentication_and_authorization
   :has_external_dependencies: 

app/workers/authorized_keys_worker.rb

+# frozen_string_literal: true
+
+class AuthorizedKeysWorker
+  include ApplicationWorker
+
+  PERMITTED_ACTIONS = [:add_key, :remove_key].freeze
+
+  feature_category :source_code_management
+  urgency :high
+  weight 2
+  idempotent!
+
+  def perform(action, *args)
+    return unless Gitlab::CurrentSettings.authorized_keys_enabled?
+
+    case action
+    when :add_key
+      authorized_keys.add_key(*args)
+    when :remove_key
+      authorized_keys.remove_key(*args)
+    end
+  end
+
+  private
+
+  def authorized_keys
+    @authorized_keys ||= Gitlab::AuthorizedKeys.new
+  end
+end

app/workers/gitlab_shell_worker.rb

@@ -9,6 +9,16 @@ class GitlabShellWorker # rubocop:disable Scalability/IdempotentWorker
   weight 2
 
   def perform(action, *arg)
+    # Gitlab::Shell is being removed but we need to continue to process jobs
+    # enqueued in the previous release, so handle them here.
+    #
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/25095 for more details
+    if AuthorizedKeysWorker::PERMITTED_ACTIONS.include?(action)
+      AuthorizedKeysWorker.new.perform(action, *arg)
+
+      return
+    end
+
     Gitlab::GitalyClient::NamespaceService.allow do
       gitlab_shell.__send__(action, *arg) # rubocop:disable GitlabSecurity/PublicSend
     end

changelogs/unreleased/25095-remove-gitlab-shell-indirection-for-authorized-keys.yml

+---
+title: Move authorized_keys operations into their own Sidekiq queue
+merge_request: 26913
+author:
+type: changed

config/sidekiq_queues.yml

@@ -30,6 +30,8 @@
   - 1
 - - analytics_code_review_metrics
   - 1
+- - authorized_keys
+  - 2
 - - authorized_projects
   - 2
 - - auto_devops

db/fixtures/development/11_keys.rb

@@ -5,9 +5,9 @@ require './spec/support/sidekiq_middleware'
 # gitlab-shell path set (yet) we need to disable this for these fixtures.
 Sidekiq::Testing.disable! do
   Gitlab::Seeder.quiet do
-    # We want to run `add_to_shell` immediately instead of after the commit, so
+    # We want to run `add_to_authorized_keys` immediately instead of after the commit, so
     # that it falls under `Sidekiq::Testing.disable!`.
-    Key.skip_callback(:commit, :after, :add_to_shell)
+    Key.skip_callback(:commit, :after, :add_to_authorized_keys)
 
     User.not_mass_generated.first(10).each do |user|
       key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
@@ -18,7 +18,7 @@ Sidekiq::Testing.disable! do
       )
 
       Sidekiq::Worker.skipping_transaction_check do
-        key.add_to_shell
+        key.add_to_authorized_keys
       end
 
       print '.'

ee/lib/ee/gitlab/background_migration/update_authorized_keys_file_since.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module BackgroundMigration
-      module UpdateAuthorizedKeysFileSince
-        extend ::Gitlab::Utils::Override
-        include ::Gitlab::ShellAdapter
-
-        class Key < ActiveRecord::Base
-          self.table_name = 'keys'
-
-          def shell_id
-            "key-#{id}"
-          end
-        end
-
-        delegate :remove_keys_not_found_in_db, to: :gitlab_shell
-
-        override :perform
-        def perform(cutoff_datetime)
-          add_keys_since(cutoff_datetime)
-
-          remove_keys_not_found_in_db
-        end
-
-        def add_keys_since(cutoff_datetime)
-          start_key = Key.select(:id).where("created_at >= ?", cutoff_datetime).order('id ASC').take
-          if start_key
-            batch_add_keys_in_db_starting_from(start_key.id)
-          end
-        end
-
-        # Not added to Gitlab::Shell because I don't expect this to be used again
-        def batch_add_keys_in_db_starting_from(start_id)
-          Rails.logger.info("Adding all keys starting from ID: #{start_id}") # rubocop:disable Gitlab/RailsLogger
-
-          Key.find_in_batches(start: start_id, batch_size: 1000) do |keys|
-            gitlab_shell.batch_add_keys(keys)
-          end
-        end
-      end
-    end
-  end
-end

ee/spec/lib/ee/gitlab/background_migration/update_authorized_keys_file_since_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-# rubocop:disable RSpec/FactoriesInMigrationSpecs
-describe Gitlab::BackgroundMigration::UpdateAuthorizedKeysFileSince, schema: :latest do
-  let(:background_migration) { described_class.new }
-
-  describe '#perform' do
-    let!(:cutoff_datetime) { DateTime.now }
-
-    subject { background_migration.perform(cutoff_datetime) }
-
-    around do |example|
-      Timecop.travel(cutoff_datetime + 1.day) { example.run }
-    end
-
-    context 'when an SSH key was created after the cutoff datetime' do
-      before do
-        @key = create(:key)
-      end
-
-      it 'calls batch_add_keys_in_db_starting_from with the start key ID' do
-        expect(background_migration).to receive(:batch_add_keys_in_db_starting_from).with(@key.id)
-        subject
-      end
-    end
-
-    it 'calls remove_keys_not_found_in_db on Gitlab::Shell' do
-      expect_next_instance_of(Gitlab::Shell) do |instance|
-        expect(instance).to receive(:remove_keys_not_found_in_db)
-      end
-      subject
-    end
-  end
-
-  describe '#add_keys_since' do
-    let!(:cutoff_datetime) { DateTime.now }
-
-    subject { background_migration.add_keys_since(cutoff_datetime) }
-
-    around do |example|
-      Timecop.travel(cutoff_datetime + 1.day) { example.run }
-    end
-
-    context 'when an SSH key was created after the cutoff datetime' do
-      before do
-        @key = create(:key)
-        create(:key) # other key
-      end
-
-      it 'calls batch_add_keys_in_db_starting_from with the start key ID' do
-        expect(background_migration).to receive(:batch_add_keys_in_db_starting_from).with(@key.id)
-        subject
-      end
-    end
-
-    context 'when an SSH key was not created after the cutoff datetime' do
-      it 'does not call batch_add_keys_in_db_starting_from' do
-        expect(background_migration).not_to receive(:batch_add_keys_in_db_starting_from)
-        subject
-      end
-    end
-  end
-
-  describe '#remove_keys_not_found_in_db' do
-    it 'calls remove_keys_not_found_in_db on Gitlab::Shell' do
-      expect_next_instance_of(Gitlab::Shell) do |instance|
-        expect(instance).to receive(:remove_keys_not_found_in_db)
-      end
-      background_migration.remove_keys_not_found_in_db
-    end
-  end
-
-  describe '#batch_add_keys_in_db_starting_from' do
-    context 'when there are many keys in the DB' do
-      before do
-        @keys = []
-        10.times do
-          @keys << create(:key)
-        end
-      end
-
-      it 'adds all the keys in the DB, starting from the given ID, to the authorized_keys file' do
-        Gitlab::Shell.new.remove_all_keys
-
-        background_migration.batch_add_keys_in_db_starting_from(@keys[3].id)
-
-        file = File.read(Rails.root.join(Gitlab.config.gitlab_shell.authorized_keys_file))
-        expect(file.scan(/ssh-rsa/).count).to eq(7)
-
-        expect(file).not_to include(strip_key(@keys[0].key))
-        expect(file).not_to include(strip_key(@keys[2].key))
-        expect(file).to include(strip_key(@keys[3].key))
-        expect(file).to include(strip_key(@keys[9].key))
-      end
-    end
-
-    def strip_key(key)
-      key.split(/[ ]+/)[0, 2].join(' ')
-    end
-  end
-end
-# rubocop:enable RSpec/FactoriesInMigrationSpecs

lib/gitlab/authorized_keys.rb

@@ -70,7 +70,7 @@ module Gitlab
     #
     # @param [String] id identifier of the key to be removed prefixed by `key-`
     # @return [Boolean]
-    def rm_key(id)
+    def remove_key(id)
       lock do
         logger.info("Removing key (#{id})")
         open_authorized_keys_file('r+') do |f|

lib/gitlab/background_migration/update_authorized_keys_file_since.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module BackgroundMigration
-    # rubocop: disable Style/Documentation
-    class UpdateAuthorizedKeysFileSince
-      def perform(cutoff_datetime)
-      end
-    end
-  end
-end
-
-Gitlab::BackgroundMigration::UpdateAuthorizedKeysFileSince.prepend_if_ee('EE::Gitlab::BackgroundMigration::UpdateAuthorizedKeysFileSince')

lib/gitlab/shell.rb

@@ -192,84 +192,6 @@ module Gitlab
       false
     end
 
-    # Add new key to authorized_keys
-    #
-    # @example Add new key
-    #   add_key("key-42", "sha-rsa ...")
-    #
-    # @param [String] key_id identifier of the key
-    # @param [String] key_content key content (public certificate)
-    # @return [Boolean] whether key could be added
-    def add_key(key_id, key_content)
-      return unless self.authorized_keys_enabled?
-
-      gitlab_authorized_keys.add_key(key_id, key_content)
-    end
-
-    # Batch-add keys to authorized_keys
-    #
-    # @example
-    #   batch_add_keys(Key.all)
-    #
-    # @param [Array<Key>] keys
-    # @return [Boolean] whether keys could be added
-    def batch_add_keys(keys)
-      return unless self.authorized_keys_enabled?
-
-      gitlab_authorized_keys.batch_add_keys(keys)
-    end
-
-    # Remove SSH key from authorized_keys
-    #
-    # @example Remove a key
-    #   remove_key("key-342")
-    #
-    # @param [String] key_id
-    # @return [Boolean] whether key could be removed or not
-    def remove_key(key_id, _ = nil)
-      return unless self.authorized_keys_enabled?
-
-      gitlab_authorized_keys.rm_key(key_id)
-    end
-
-    # Remove all SSH keys from gitlab shell
-    #
-    # @example Remove all keys
-    #   remove_all_keys
-    #
-    # @return [Boolean] whether keys could be removed or not
-    def remove_all_keys
-      return unless self.authorized_keys_enabled?
-
-      gitlab_authorized_keys.clear
-    end
-
-    # Remove SSH keys from gitlab shell that are not in the DB
-    #
-    # @example Remove keys not on the database
-    #   remove_keys_not_found_in_db
-    #
-    # rubocop: disable CodeReuse/ActiveRecord
-    def remove_keys_not_found_in_db
-      return unless self.authorized_keys_enabled?
-
-      Rails.logger.info("Removing keys not found in DB") # rubocop:disable Gitlab/RailsLogger
-
-      batch_read_key_ids do |ids_in_file|
-        ids_in_file.uniq!
-        keys_in_db = Key.where(id: ids_in_file)
-
-        next unless ids_in_file.size > keys_in_db.count # optimization
-
-        ids_to_remove = ids_in_file - keys_in_db.pluck(:id)
-        ids_to_remove.each do |id|
-          Rails.logger.info("Removing key-#{id} not found in DB") # rubocop:disable Gitlab/RailsLogger
-          remove_key("key-#{id}")
-        end
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     # Add empty directory for storing repositories
     #
     # @example Add new namespace directory
@@ -372,14 +294,6 @@ module Gitlab
       File.join(Gitlab.config.repositories.storages[storage].legacy_disk_path, dir_name)
     end
 
-    def authorized_keys_enabled?
-      # Return true if nil to ensure the authorized_keys methods work while
-      # fixing the authorized_keys file during migration.
-      return true if Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled.nil?
-
-      Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled
-    end
-
     private
 
     def git_timeout
@@ -394,32 +308,6 @@ module Gitlab
       raise Error, e
     end
 
-    def gitlab_authorized_keys
-      @gitlab_authorized_keys ||= Gitlab::AuthorizedKeys.new
-    end
-
-    def batch_read_key_ids(batch_size: 100, &block)
-      return unless self.authorized_keys_enabled?
-
-      gitlab_authorized_keys.list_key_ids.lazy.each_slice(batch_size) do |key_ids|
-        yield(key_ids)
-      end
-    end
-
-    def strip_key(key)
-      key.split(/[ ]+/)[0, 2].join(' ')
-    end
-
-    def add_keys_to_io(keys, io)
-      keys.each do |k|
-        key = strip_key(k.key)
-
-        raise Error.new("Invalid key: #{key.inspect}") if key.include?("\t") || key.include?("\n")
-
-        io.puts("#{k.shell_id}\t#{key}")
-      end
-    end
-
     class GitalyGitlabProjects
       attr_reader :shard_name, :repository_relative_path, :output, :gl_project_path
 

lib/tasks/gitlab/shell.rake

@@ -89,10 +89,12 @@ namespace :gitlab do
       puts ""
     end
 
-    Gitlab::Shell.new.remove_all_keys
+    authorized_keys = Gitlab::AuthorizedKeys.new
+
+    authorized_keys.clear
 
     Key.find_in_batches(batch_size: 1000) do |keys|
-      unless Gitlab::Shell.new.batch_add_keys(keys)
+      unless authorized_keys.batch_add_keys(keys)
         puts "Failed to add keys...".color(:red)
         exit 1
       end
@@ -103,7 +105,7 @@ namespace :gitlab do
   end
 
   def ensure_write_to_authorized_keys_is_enabled
-    return if Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled
+    return if Gitlab::CurrentSettings.authorized_keys_enabled?
 
     puts authorized_keys_is_disabled_warning
 
@@ -113,7 +115,7 @@ namespace :gitlab do
     end
 
     puts 'Enabling the "Write to authorized_keys file" setting...'
-    Gitlab::CurrentSettings.current_application_settings.update!(authorized_keys_enabled: true)
+    Gitlab::CurrentSettings.update!(authorized_keys_enabled: true)
 
     puts 'Successfully enabled "Write to authorized_keys file"!'
     puts ''

spec/lib/gitlab/authorized_keys_spec.rb

@@ -162,10 +162,10 @@ describe Gitlab::AuthorizedKeys do
     end
   end
 
-  describe '#rm_key' do
+  describe '#remove_key' do
     let(:key) { 'key-741' }
 
-    subject { authorized_keys.rm_key(key) }
+    subject { authorized_keys.remove_key(key) }
 
     context 'authorized_keys file exists' do
       let(:other_line) { "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-742\",options ssh-rsa AAAAB3NzaDAxx2E" }

spec/lib/gitlab/shell_spec.rb

@@ -9,14 +9,11 @@ describe Gitlab::Shell do
   let(:gitlab_shell) { described_class.new }
   let(:popen_vars) { { 'GIT_TERMINAL_PROMPT' => ENV['GIT_TERMINAL_PROMPT'] } }
   let(:timeout) { Gitlab.config.gitlab_shell.git_timeout }
-  let(:gitlab_authorized_keys) { double }
 
   before do
     allow(Project).to receive(:find).and_return(project)
   end
 
-  it { is_expected.to respond_to :add_key }
-  it { is_expected.to respond_to :remove_key }
   it { is_expected.to respond_to :create_repository }
   it { is_expected.to respond_to :remove_repository }
   it { is_expected.to respond_to :fork_repository }
@@ -49,227 +46,6 @@ describe Gitlab::Shell do
     end
   end
 
-  describe '#add_key' do
-    context 'when authorized_keys_enabled is true' do
-      it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-
-        expect(gitlab_authorized_keys)
-          .to receive(:add_key)
-          .with('key-123', 'ssh-rsa foobar')
-
-        gitlab_shell.add_key('key-123', 'ssh-rsa foobar')
-      end
-    end
-
-    context 'when authorized_keys_enabled is false' do
-      before do
-        stub_application_setting(authorized_keys_enabled: false)
-      end
-
-      it 'does nothing' do
-        expect(Gitlab::AuthorizedKeys).not_to receive(:new)
-
-        gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
-      end
-    end
-
-    context 'when authorized_keys_enabled is nil' do
-      before do
-        stub_application_setting(authorized_keys_enabled: nil)
-      end
-
-      it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-
-        expect(gitlab_authorized_keys)
-          .to receive(:add_key)
-          .with('key-123', 'ssh-rsa foobar')
-
-        gitlab_shell.add_key('key-123', 'ssh-rsa foobar')
-      end
-    end
-  end
-
-  describe '#batch_add_keys' do
-    let(:keys) { [double(shell_id: 'key-123', key: 'ssh-rsa foobar')] }
-
-    context 'when authorized_keys_enabled is true' do
-      it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-
-        expect(gitlab_authorized_keys)
-          .to receive(:batch_add_keys)
-          .with(keys)
-
-        gitlab_shell.batch_add_keys(keys)
-      end
-    end
-
-    context 'when authorized_keys_enabled is false' do
-      before do
-        stub_application_setting(authorized_keys_enabled: false)
-      end
-
-      it 'does nothing' do
-        expect(Gitlab::AuthorizedKeys).not_to receive(:new)
-
-        gitlab_shell.batch_add_keys(keys)
-      end
-    end
-
-    context 'when authorized_keys_enabled is nil' do
-      before do
-        stub_application_setting(authorized_keys_enabled: nil)
-      end
-
-      it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-
-        expect(gitlab_authorized_keys)
-          .to receive(:batch_add_keys)
-          .with(keys)
-
-        gitlab_shell.batch_add_keys(keys)
-      end
-    end
-  end
-
-  describe '#remove_key' do
-    context 'when authorized_keys_enabled is true' do
-      it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-        expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
-
-        gitlab_shell.remove_key('key-123')
-      end
-    end
-
-    context 'when authorized_keys_enabled is false' do
-      before do
-        stub_application_setting(authorized_keys_enabled: false)
-      end
-
-      it 'does nothing' do
-        expect(Gitlab::AuthorizedKeys).not_to receive(:new)
-
-        gitlab_shell.remove_key('key-123')
-      end
-    end
-
-    context 'when authorized_keys_enabled is nil' do
-      before do
-        stub_application_setting(authorized_keys_enabled: nil)
-      end
-
-      it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-        expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
-
-        gitlab_shell.remove_key('key-123')
-      end
-    end
-  end
-
-  describe '#remove_all_keys' do
-    context 'when authorized_keys_enabled is true' do
-      it 'calls Gitlab::AuthorizedKeys#clear' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-        expect(gitlab_authorized_keys).to receive(:clear)
-
-        gitlab_shell.remove_all_keys
-      end
-    end
-
-    context 'when authorized_keys_enabled is false' do
-      before do
-        stub_application_setting(authorized_keys_enabled: false)
-      end
-
-      it 'does nothing' do
-        expect(Gitlab::AuthorizedKeys).not_to receive(:new)
-
-        gitlab_shell.remove_all_keys
-      end
-    end
-
-    context 'when authorized_keys_enabled is nil' do
-      before do
-        stub_application_setting(authorized_keys_enabled: nil)
-      end
-
-      it 'calls Gitlab::AuthorizedKeys#clear' do
-        expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
-        expect(gitlab_authorized_keys).to receive(:clear)
-
-        gitlab_shell.remove_all_keys
-      end
-    end
-  end
-
-  describe '#remove_keys_not_found_in_db' do
-    context 'when keys are in the file that are not in the DB' do
-      before do
-        gitlab_shell.remove_all_keys
-        gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-        gitlab_shell.add_key('key-9876', 'ssh-rsa ASDFASDF')
-        @another_key = create(:key) # this one IS in the DB
-      end
-
-      it 'removes the keys' do
-        expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-        expect(gitlab_shell).to receive(:remove_key).with('key-9876')
-        expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@another_key.id}")
-
-        gitlab_shell.remove_keys_not_found_in_db
-      end
-    end
-
-    context 'when keys there are duplicate keys in the file that are not in the DB' do
-      before do
-        gitlab_shell.remove_all_keys
-        gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-        gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-      end
-
-      it 'removes the keys' do
-        expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-
-        gitlab_shell.remove_keys_not_found_in_db
-      end
-    end
-
-    context 'when keys there are duplicate keys in the file that ARE in the DB' do
-      before do
-        gitlab_shell.remove_all_keys
-        @key = create(:key)
-        gitlab_shell.add_key(@key.shell_id, @key.key)
-      end
-
-      it 'does not remove the key' do
-        expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@key.id}")
-
-        gitlab_shell.remove_keys_not_found_in_db
-      end
-    end
-
-    unless ENV['CI'] # Skip in CI, it takes 1 minute
-      context 'when the first batch can be skipped, but the next batch has keys that are not in the DB' do
-        before do
-          gitlab_shell.remove_all_keys
-          100.times { |i| create(:key) } # first batch is all in the DB
-          gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
-        end
-
-        it 'removes the keys not in the DB' do
-          expect(gitlab_shell).to receive(:remove_key).with('key-1234')
-
-          gitlab_shell.remove_keys_not_found_in_db
-        end
-      end
-    end
-  end
-
   describe 'projects commands' do
     let(:gitlab_shell_path) { File.expand_path('tmp/tests/gitlab-shell') }
     let(:projects_path) { File.join(gitlab_shell_path, 'bin/gitlab-projects') }

spec/models/key_spec.rb

@@ -181,19 +181,52 @@ describe Key, :mailer do
   end
 
   context 'callbacks' do
+    let(:key) { build(:personal_key) }
+
+    context 'authorized keys file is enabled' do
+      before do
+        stub_application_setting(authorized_keys_enabled: true)
+      end
+
       it 'adds new key to authorized_file' do
-      key = build(:personal_key, id: 7)
-      expect(GitlabShellWorker).to receive(:perform_async).with(:add_key, key.shell_id, key.key)
+        allow(AuthorizedKeysWorker).to receive(:perform_async)
+
         key.save!
+
+        # Check after the fact so we have access to Key#id
+        expect(AuthorizedKeysWorker).to have_received(:perform_async).with(:add_key, key.shell_id, key.key)
       end
 
       it 'removes key from authorized_file' do
-      key = create(:personal_key)
-      expect(GitlabShellWorker).to receive(:perform_async).with(:remove_key, key.shell_id)
+        key.save!
+
+        expect(AuthorizedKeysWorker).to receive(:perform_async).with(:remove_key, key.shell_id)
+
         key.destroy
       end
     end
 
+    context 'authorized_keys file is disabled' do
+      before do
+        stub_application_setting(authorized_keys_enabled: false)
+      end
+
+      it 'does not add the key on creation' do
+        expect(AuthorizedKeysWorker).not_to receive(:perform_async)
+
+        key.save!
+      end
+
+      it 'does not remove the key on destruction' do
+        key.save!
+
+        expect(AuthorizedKeysWorker).not_to receive(:perform_async)
+
+        key.destroy
+      end
+    end
+  end
+
   describe '#key=' do
     let(:valid_key) do
       "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0= dummy@gitlab.com"

spec/workers/authorized_keys_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AuthorizedKeysWorker do
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    context 'authorized_keys is enabled' do
+      before do
+        stub_application_setting(authorized_keys_enabled: true)
+      end
+
+      describe '#add_key' do
+        it 'delegates to Gitlab::AuthorizedKeys' do
+          expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
+            expect(instance).to receive(:add_key).with('foo', 'bar')
+          end
+
+          worker.perform(:add_key, 'foo', 'bar')
+        end
+      end
+
+      describe '#remove_key' do
+        it 'delegates to Gitlab::AuthorizedKeys' do
+          expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
+            expect(instance).to receive(:remove_key).with('foo', 'bar')
+          end
+
+          worker.perform(:remove_key, 'foo', 'bar')
+        end
+      end
+
+      describe 'all other commands' do
+        it 'does nothing' do
+          expect(Gitlab::AuthorizedKeys).not_to receive(:new)
+
+          worker.perform(:foo, 'bar', 'baz')
+        end
+      end
+    end
+
+    context 'authorized_keys is disabled' do
+      before do
+        stub_application_setting(authorized_keys_enabled: false)
+      end
+
+      it 'does nothing' do
+        expect(Gitlab::AuthorizedKeys).not_to receive(:new)
+
+        worker.perform(:add_key, 'foo', 'bar')
+      end
+    end
+  end
+end

spec/workers/gitlab_shell_worker_spec.rb

@@ -5,12 +5,35 @@ require 'spec_helper'
 describe GitlabShellWorker do
   let(:worker) { described_class.new }
 
-  describe '#perform with add_key' do
-    it 'calls add_key on Gitlab::Shell' do
-      expect_next_instance_of(Gitlab::Shell) do |instance|
+  describe '#perform' do
+    describe '#add_key' do
+      it 'delegates to Gitlab::AuthorizedKeys' do
+        expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
           expect(instance).to receive(:add_key).with('foo', 'bar')
         end
+
         worker.perform(:add_key, 'foo', 'bar')
       end
     end
+
+    describe '#remove_key' do
+      it 'delegates to Gitlab::AuthorizedKeys' do
+        expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
+          expect(instance).to receive(:remove_key).with('foo', 'bar')
+        end
+
+        worker.perform(:remove_key, 'foo', 'bar')
+      end
+    end
+
+    describe 'all other commands' do
+      it 'delegates them to Gitlab::Shell' do
+        expect_next_instance_of(Gitlab::Shell) do |instance|
+          expect(instance).to receive(:foo).with('bar', 'baz')
+        end
+
+        worker.perform(:foo, 'bar', 'baz')
+      end
+    end
+  end
 end