Separate SSH host keys from ProjectImportData

As we're using SSH host key detection for both pull and push mirroring,
we can no longer just look the previous state of the host keys up in
the database.

Instead, the frontend can pass the value it currently knows about to
the controller, which can respond with the information we're interested
in - whether the keys it has detected differ from the keys we currently
know about.

app/controllers/projects/mirrors_controller.rb

@@ -47,7 +47,7 @@ class Projects::MirrorsController < Projects::ApplicationController
   end
 
   def ssh_host_keys
-    lookup = SshHostKey.new(project: project, url: params[:ssh_url])
+    lookup = SshHostKey.new(project: project, url: params[:ssh_url], compare_host_keys: params[:compare_host_keys])
 
     if lookup.error.present?
       # Failed to read keys

app/models/ssh_host_key.rb

+# frozen_string_literal: true
+
 # Detected SSH host keys are transiently stored in Redis
 class SshHostKey
   class Fingerprint < Gitlab::SSHPublicKey
@@ -41,11 +43,12 @@ class SshHostKey
       .select(&:valid?)
   end
 
-  attr_reader :project, :url
+  attr_reader :project, :url, :compare_host_keys
 
-  def initialize(project:, url:)
+  def initialize(project:, url:, compare_host_keys: nil)
     @project = project
     @url = normalize_url(url)
+    @compare_host_keys = compare_host_keys
   end
 
   def id
@@ -54,7 +57,7 @@ class SshHostKey
 
   def as_json(*)
     {
-      changes_project_import_data: changes_project_import_data?,
+      host_keys_changed: host_keys_changed?,
       fingerprints: fingerprints,
       known_hosts: known_hosts
     }
@@ -68,15 +71,10 @@ class SshHostKey
     @fingerprints ||= self.class.fingerprint_host_keys(known_hosts)
   end
 
-  # Returns true if the known_hosts data differs from that currently set for
-  # `project.import_data.ssh_known_hosts`. Ordering is ignored.
-  #
-  # Ordering is ignored
-  def changes_project_import_data?
-    our_known_hosts = known_hosts
-    project_known_hosts = project.import_data&.ssh_known_hosts
-
-    cleanup(our_known_hosts.to_s) != cleanup(project_known_hosts.to_s)
+  # Returns true if the known_hosts data differs from the version passed in at
+  # initialization as `compare_host_keys`. Comments, ordering, etc, is ignored
+  def host_keys_changed?
+    cleanup(known_hosts) != cleanup(compare_host_keys)
   end
 
   def error
@@ -112,6 +110,7 @@ class SshHostKey
   # Remove comments and duplicate entries
   def cleanup(data)
     data
+      .to_s
       .each_line
       .map { |line| line unless line.start_with?('#') || line.chomp.empty? }
       .compact

ee/app/assets/javascripts/mirrors/mirror_pull.js

@@ -13,6 +13,7 @@ export default class MirrorPull {
     this.$form = $(formSelector);
 
     this.$repositoryUrl = this.$form.find('.js-repo-url');
+    this.$knownHosts = this.$form.find('.js-known-hosts');
 
     this.$sectionSSHHostKeys = this.$form.find('.js-ssh-host-keys-section');
     this.$hostKeysInformation = this.$form.find('.js-fingerprint-ssh-info');
@@ -34,7 +35,7 @@ export default class MirrorPull {
     this.handleRepositoryUrlInput(true);
 
     this.$repositoryUrl.on('keyup', () => this.handleRepositoryUrlInput());
-    this.$form.find('.js-known-hosts').on('keyup', e => this.handleSSHKnownHostsInput(e));
+    this.$knownHosts.on('keyup', e => this.handleSSHKnownHostsInput(e));
     this.$dropdownAuthType.on('change', e => this.handleAuthTypeChange(e));
     this.$btnDetectHostKeys.on('click', e => this.handleDetectHostKeys(e));
     this.$btnSSHHostsShowAdvanced.on('click', e => this.handleSSHHostsAdvanced(e));
@@ -85,6 +86,7 @@ export default class MirrorPull {
   handleDetectHostKeys() {
     const projectMirrorSSHEndpoint = this.$form.data('project-mirror-ssh-endpoint');
     const repositoryUrl = this.$repositoryUrl.val();
+    const currentKnownHosts = this.$knownHosts.val();
     const $btnLoadSpinner = this.$btnDetectHostKeys.find('.js-spinner');
 
     // Disable button while we make request
@@ -94,7 +96,7 @@ export default class MirrorPull {
     // Make backOff polling to get data
     backOff((next, stop) => {
       axios
-        .get(`${projectMirrorSSHEndpoint}?ssh_url=${repositoryUrl}`)
+        .get(`${projectMirrorSSHEndpoint}?ssh_url=${repositoryUrl}&compare_host_keys=${encodeURIComponent(currentKnownHosts)}`)
         .then(({ data, status }) => {
           if (status === 204) {
             this.backOffRequestCounter += 1;
@@ -114,7 +116,7 @@ export default class MirrorPull {
         // Once data is received, we show verification info along with Host keys and fingerprints
         this.$hostKeysInformation
           .find('.js-fingerprint-verification')
-          .collapse(res.changes_project_import_data ? 'hide' : 'show');
+          .collapse(res.host_keys_changed ? 'hide' : 'show');
         if (res.known_hosts && res.fingerprints) {
           this.showSSHInformation(res);
         }

spec/controllers/projects/mirrors_controller_spec.rb

@@ -115,7 +115,7 @@ describe Projects::MirrorsController do
         do_get(project)
 
         expect(response).to have_gitlab_http_status(200)
-        expect(json_response).to eq('known_hosts' => ssh_key, 'fingerprints' => [ssh_fp.stringify_keys], 'changes_project_import_data' => true)
+        expect(json_response).to eq('known_hosts' => ssh_key, 'fingerprints' => [ssh_fp.stringify_keys], 'host_keys_changed' => true)
       end
     end
 

spec/models/ssh_host_key_spec.rb

@@ -33,6 +33,8 @@ describe SshHostKey do
   let(:extra) { known_hosts + "foo\nbar\n" }
   let(:reversed) { known_hosts.lines.reverse.join }
 
+  let(:compare_host_keys) { nil }
+
   def stub_ssh_keyscan(args, status: true, stdout: "", stderr: "")
     stdin = StringIO.new
     stdout = double(:stdout, read: stdout)
@@ -44,9 +46,9 @@ describe SshHostKey do
     stdin
   end
 
-  let(:project) { build(:project, :mirror) }
+  let(:project) { build(:project) }
 
-  subject(:ssh_host_key) { described_class.new(project: project, url: 'ssh://example.com:2222') }
+  subject(:ssh_host_key) { described_class.new(project: project, url: 'ssh://example.com:2222', compare_host_keys: compare_host_keys) }
 
   describe '#fingerprints', :use_clean_rails_memory_store_caching do
     it 'returns an array of indexed fingerprints when the cache is filled' do
@@ -82,7 +84,7 @@ describe SshHostKey do
     end
   end
 
-  describe '#changes_project_import_data?' do
+  describe '#host_keys_changed?' do
     where(:a, :b, :result) do
       known_hosts | extra       | true
       known_hosts | "foo\n"     | true
@@ -97,21 +99,29 @@ describe SshHostKey do
     end
 
     with_them do
-      subject { ssh_host_key.changes_project_import_data? }
+      let(:compare_host_keys) { b }
+
+      subject { ssh_host_key.host_keys_changed? }
 
-      it "(normal)" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(a)
-        project.import_data.ssh_known_hosts = b
+      context '(normal)' do
+        let(:compare_host_keys) { b }
 
-        is_expected.to eq(result)
+        before do
+          expect(ssh_host_key).to receive(:known_hosts).and_return(a)
+        end
+
+        it { is_expected.to eq(result) }
       end
 
       # Comparisons should be symmetrical, so test the reverse too
-      it "(reversed)" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(b)
-        project.import_data.ssh_known_hosts = a
+      context '(reversed)' do
+        let(:compare_host_keys) { a }
+
+        before do
+          expect(ssh_host_key).to receive(:known_hosts).and_return(b)
+        end
 
-        is_expected.to eq(result)
+        it { is_expected.to eq(result) }
       end
     end
   end