Only create commit GPG signature when necessary

4 files changed, 84 insertions, 70 deletions

diff --git a/spec/lib/gitlab/gpg/commit_spec.rb b/spec/lib/gitlab/gpg/commit_spec.rb
index ddb8dd9f0f4..e521fcc6dc1 100644
--- a/spec/lib/gitlab/gpg/commit_spec.rb
+++ b/spec/lib/gitlab/gpg/commit_spec.rb
@@ -1,13 +1,13 @@
 require 'rails_helper'
 
-RSpec.describe Gitlab::Gpg::Commit do
+describe Gitlab::Gpg::Commit do
   describe '#signature' do
     let!(:project) { create :project, :repository, path: 'sample-project' }
     let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'  }
 
-    context 'unisgned commit' do
+    context 'unsigned commit' do
       it 'returns nil' do
-        expect(described_class.new(project.commit).signature).to be_nil
+        expect(described_class.new(project, commit_sha).signature).to be_nil
       end
     end
 
@@ -16,18 +16,19 @@ RSpec.describe Gitlab::Gpg::Commit do
         create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
       end
 
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
-          GpgHelpers::User1.signed_commit_signature,
-          GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit, git_commit: raw_commit, project: project
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
+              GpgHelpers::User1.signed_commit_signature,
+              GpgHelpers::User1.signed_commit_base_data
+            ]
+          )
       end
 
       it 'returns a valid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: gpg_key,
@@ -39,7 +40,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature
@@ -53,18 +54,19 @@ RSpec.describe Gitlab::Gpg::Commit do
     context 'known but unverified public key' do
       let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key }
 
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
-          GpgHelpers::User1.signed_commit_signature,
-          GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit, git_commit: raw_commit, project: project
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
+              GpgHelpers::User1.signed_commit_signature,
+              GpgHelpers::User1.signed_commit_base_data
+            ]
+          )
       end
 
       it 'returns an invalid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: gpg_key,
@@ -76,7 +78,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature
@@ -88,20 +90,19 @@ RSpec.describe Gitlab::Gpg::Commit do
     end
 
     context 'unknown public key' do
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
-          GpgHelpers::User1.signed_commit_signature,
-          GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit,
-          git_commit: raw_commit,
-          project: project
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
+              GpgHelpers::User1.signed_commit_signature,
+              GpgHelpers::User1.signed_commit_base_data
+            ]
+          )
       end
 
       it 'returns an invalid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: nil,
@@ -113,7 +114,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature
diff --git a/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb b/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
index c4e04ee46a2..4de4419de27 100644
--- a/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
+++ b/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
@@ -4,23 +4,16 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
   describe '#run' do
     let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
     let!(:project) { create :project, :repository, path: 'sample-project' }
-    let!(:raw_commit) do
-      raw_commit = double(:raw_commit, signature: [
-        GpgHelpers::User1.signed_commit_signature,
-        GpgHelpers::User1.signed_commit_base_data
-      ], sha: commit_sha)
-
-      allow(raw_commit).to receive :save!
-
-      raw_commit
-    end
-
-    let!(:commit) do
-      create :commit, git_commit: raw_commit, project: project
-    end
 
     before do
-      allow_any_instance_of(Project).to receive(:commit).and_return(commit)
+      allow(Rugged::Commit).to receive(:extract_signature)
+        .with(Rugged::Repository, commit_sha)
+        .and_return(
+          [
+            GpgHelpers::User1.signed_commit_signature,
+            GpgHelpers::User1.signed_commit_base_data
+          ]
+        )
     end
 
     context 'gpg signature did have an associated gpg key which was removed later' do
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 8485605b398..e3c1bdce300 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -688,10 +688,38 @@ describe GitPushService, services: true do
       )
     end
 
-    it 'calls CreateGpgSignatureWorker.perform_async for each commit' do
-      expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id)
+    context 'when the commit has a signature' do
+      context 'when the signature is already cached' do
+        before do
+          create(:gpg_signature, commit_sha: sample_commit.id)
+        end
 
-      execute_service(project, user, oldrev, newrev, ref)
+        it 'does not queue a CreateGpgSignatureWorker' do
+          expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
+
+          execute_service(project, user, oldrev, newrev, ref)
+        end
+      end
+
+      context 'when the signature is not yet cached' do
+        it 'queues a CreateGpgSignatureWorker' do
+          expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id)
+
+          execute_service(project, user, oldrev, newrev, ref)
+        end
+      end
+    end
+
+    context 'when the commit does not have a signature' do
+      before do
+        allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).with(project.repository, [sample_commit.id]).and_return([])
+      end
+
+      it 'does not queue a CreateGpgSignatureWorker' do
+        expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
+
+        execute_service(project, user, oldrev, newrev, ref)
+      end
     end
   end
 
diff --git a/spec/workers/create_gpg_signature_worker_spec.rb b/spec/workers/create_gpg_signature_worker_spec.rb
index c6a17d77d73..54978baca88 100644
--- a/spec/workers/create_gpg_signature_worker_spec.rb
+++ b/spec/workers/create_gpg_signature_worker_spec.rb
@@ -1,34 +1,26 @@
 require 'spec_helper'
 
 describe CreateGpgSignatureWorker do
+  let(:project) { create(:project, :repository) }
+
   context 'when GpgKey is found' do
-    it 'calls Commit#signature' do
-      commit_sha = '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'
-      project = create :project
-      commit = instance_double(Commit)
+    let(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
 
-      allow(Project).to receive(:find_by).with(id: project.id).and_return(project)
-      allow(project).to receive(:commit).with(commit_sha).and_return(commit)
+    it 'calls Gitlab::Gpg::Commit#signature' do
+      expect(Gitlab::Gpg::Commit).to receive(:new).with(project, commit_sha).and_call_original
 
-      expect(commit).to receive(:signature)
+      expect_any_instance_of(Gitlab::Gpg::Commit).to receive(:signature)
 
       described_class.new.perform(commit_sha, project.id)
     end
   end
 
   context 'when Commit is not found' do
-    let(:nonexisting_commit_sha) { 'bogus' }
-    let(:project) { create :project }
+    let(:nonexisting_commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a34' }
 
     it 'does not raise errors' do
       expect { described_class.new.perform(nonexisting_commit_sha, project.id) }.not_to raise_error
     end
-
-    it 'does not call Commit#signature' do
-      expect_any_instance_of(Commit).not_to receive(:signature)
-
-      described_class.new.perform(nonexisting_commit_sha, project.id)
-    end
   end
 
   context 'when Project is not found' do
@@ -38,8 +30,8 @@ describe CreateGpgSignatureWorker do
       expect { described_class.new.perform(anything, nonexisting_project_id) }.not_to raise_error
     end
 
-    it 'does not call Commit#signature' do
-      expect_any_instance_of(Commit).not_to receive(:signature)
+    it 'does not call Gitlab::Gpg::Commit#signature' do
+      expect_any_instance_of(Gitlab::Gpg::Commit).not_to receive(:signature)
 
       described_class.new.perform(anything, nonexisting_project_id)
     end