Retrieve commit signatures with Gitaly

Author: jacobvosmaer

GITALY_SERVER_VERSION

@@ -1 +1 @@
-0.69.0
+0.70.0

app/controllers/projects/commits_controller.rb

@@ -13,31 +13,37 @@ def show
     @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
       .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
 
-    respond_to do |format|
-      format.html
-      format.atom { render layout: 'xml.atom' }
-
-      format.json do
-        pager_json(
-          'projects/commits/_commits',
-          @commits.size,
-          project: @project,
-          ref: @ref)
+    # https://gitlab.com/gitlab-org/gitaly/issues/931
+    Gitlab::GitalyClient.allow_n_plus_1_calls do
+      respond_to do |format|
+        format.html
+        format.atom { render layout: 'xml.atom' }
+
+        format.json do
+          pager_json(
+            'projects/commits/_commits',
+            @commits.size,
+            project: @project,
+            ref: @ref)
+        end
       end
     end
   end
 
   def signatures
-    respond_to do |format|
-      format.json do
-        render json: {
-          signatures: @commits.select(&:has_signature?).map do |commit|
-            {
-              commit_sha: commit.sha,
-              html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
-            }
-          end
-        }
+    # https://gitlab.com/gitlab-org/gitaly/issues/931
+    Gitlab::GitalyClient.allow_n_plus_1_calls do
+      respond_to do |format|
+        format.json do
+          render json: {
+            signatures: @commits.select(&:has_signature?).map do |commit|
+              {
+                commit_sha: commit.sha,
+                html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
+              }
+            end
+          }
+        end
       end
     end
   end

lib/gitlab/git/commit.rb

@@ -239,6 +239,24 @@ def batch_by_oid(repo, oids)
             end
           end
         end
+
+        def extract_signature(repository, commit_id)
+          repository.gitaly_migrate(:extract_commit_signature) do |is_enabled|
+            if is_enabled
+              repository.gitaly_commit_client.extract_signature(commit_id)
+            else
+              rugged_extract_signature(repository, commit_id)
+            end
+          end
+        end
+
+        def rugged_extract_signature(repository, commit_id)
+          begin
+            Rugged::Commit.extract_signature(repository.rugged, commit_id)
+          rescue Rugged::OdbError
+            nil
+          end
+        end
       end
 
       def initialize(repository, raw_commit, head = nil)

lib/gitlab/gitaly_client/commit_service.rb

@@ -282,6 +282,23 @@ def filter_shas_with_signatures(shas)
         end
       end
 
+      def extract_signature(commit_id)
+        request = Gitaly::ExtractCommitSignatureRequest.new(repository: @gitaly_repo, commit_id: commit_id)
+        response = GitalyClient.call(@repository.storage, :commit_service, :extract_commit_signature, request)
+
+        signature = ''.b
+        signed_text = ''.b
+
+        response.each do |message|
+          signature << message.signature
+          signed_text << message.signed_text
+        end
+
+        return if signature.blank? && signed_text.blank?
+
+        [signature, signed_text]
+      end
+
       private
 
       def call_commit_diff(request_params, options = {})

lib/gitlab/gpg/commit.rb

@@ -4,12 +4,8 @@ class Commit
       def initialize(commit)
         @commit = commit
 
-        @signature_text, @signed_text =
-          begin
-            Rugged::Commit.extract_signature(@commit.project.repository.rugged, @commit.sha)
-          rescue Rugged::OdbError
-            nil
-          end
+        repo = commit.project.repository.raw_repository
+        @signature_text, @signed_text = Gitlab::Git::Commit.extract_signature(repo, commit.sha)
       end
 
       def has_signature?

spec/lib/gitlab/git/commit_spec.rb

@@ -388,6 +388,84 @@
         end
       end
     end
+
+    describe '.extract_signature' do
+      subject { described_class.extract_signature(repository, commit_id) }
+
+      shared_examples '.extract_signature' do
+        context 'when the commit is signed' do
+          let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' }
+
+          it 'returns signature and signed text' do
+            signature, signed_text = subject
+
+            expected_signature = <<~SIGNATURE
+              -----BEGIN PGP SIGNATURE-----
+              Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
+              Comment: GPGTools - https://gpgtools.org
+
+              iQEcBAABCgAGBQJTDvaZAAoJEGJ8X1ifRn8XfvYIAMuB0yrbTGo1BnOSoDfyrjb0
+              Kw2EyUzvXYL72B63HMdJ+/0tlSDC6zONF3fc+bBD8z+WjQMTbwFNMRbSSy2rKEh+
+              mdRybOP3xBIMGgEph0/kmWln39nmFQBsPRbZBWoU10VfI/ieJdEOgOphszgryRar
+              TyS73dLBGE9y9NIININVaNISet9D9QeXFqc761CGjh4YIghvPpi+YihMWapGka6v
+              hgKhX+hc5rj+7IEE0CXmlbYR8OYvAbAArc5vJD7UTxAY4Z7/l9d6Ydt9GQ25khfy
+              ANFgltYzlR6evLFmDjssiP/mx/ZMN91AL0ueJ9nNGv411Mu2CUW+tDCaQf35mdc=
+              =j51i
+              -----END PGP SIGNATURE-----
+            SIGNATURE
+
+            expect(signature).to eq(expected_signature.chomp)
+            expect(signature).to be_a_binary_string
+
+            expected_signed_text = <<~SIGNED_TEXT
+              tree 22bfa2fbd217df24731f43ff43a4a0f8db759dae
+              parent ae73cb07c9eeaf35924a10f713b364d32b2dd34f
+              author Dmitriy Zaporozhets <[email protected]> 1393489561 +0200
+              committer Dmitriy Zaporozhets <[email protected]> 1393489561 +0200
+
+              Feature added
+
+              Signed-off-by: Dmitriy Zaporozhets <[email protected]>
+            SIGNED_TEXT
+
+            expect(signed_text).to eq(expected_signed_text)
+            expect(signed_text).to be_a_binary_string
+          end
+        end
+
+        context 'when the commit has no signature' do
+          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'when the commit cannot be found' do
+          let(:commit_id) { Gitlab::Git::BLANK_SHA }
+
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+
+        context 'when the commit ID is invalid' do
+          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
+
+          it 'raises ArgumentError' do
+            expect { subject }.to raise_error(ArgumentError)
+          end
+        end
+      end
+
+      context 'with gitaly' do
+        it_behaves_like '.extract_signature'
+      end
+
+      context 'without gitaly', :skip_gitaly_mock do
+        it_behaves_like '.extract_signature'
+      end
+    end
   end
 
   describe '#init_from_rugged' do

spec/lib/gitlab/gpg/commit_spec.rb

@@ -38,8 +38,8 @@
           end
 
           before do
-            allow(Rugged::Commit).to receive(:extract_signature)
-            .with(Rugged::Repository, commit_sha)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
                 GpgHelpers::User1.signed_commit_signature,
@@ -77,8 +77,8 @@
           end
 
           before do
-            allow(Rugged::Commit).to receive(:extract_signature)
-            .with(Rugged::Repository, commit_sha)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
                 GpgHelpers::User3.signed_commit_signature,
@@ -116,8 +116,8 @@
           end
 
           before do
-            allow(Rugged::Commit).to receive(:extract_signature)
-            .with(Rugged::Repository, commit_sha)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
                 GpgHelpers::User1.signed_commit_signature,
@@ -151,8 +151,8 @@
           end
 
           before do
-            allow(Rugged::Commit).to receive(:extract_signature)
-            .with(Rugged::Repository, commit_sha)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
                 GpgHelpers::User1.signed_commit_signature,
@@ -187,8 +187,8 @@
         end
 
         before do
-          allow(Rugged::Commit).to receive(:extract_signature)
-          .with(Rugged::Repository, commit_sha)
+          allow(Gitlab::Git::Commit).to receive(:extract_signature)
+          .with(Gitlab::Git::Repository, commit_sha)
           .and_return(
             [
               GpgHelpers::User1.signed_commit_signature,
@@ -217,8 +217,8 @@
       let!(:commit) { create :commit, project: project, sha: commit_sha }
 
       before do
-        allow(Rugged::Commit).to receive(:extract_signature)
-          .with(Rugged::Repository, commit_sha)
+        allow(Gitlab::Git::Commit).to receive(:extract_signature)
+          .with(Gitlab::Git::Repository, commit_sha)
           .and_return(
             [
               GpgHelpers::User1.signed_commit_signature,

spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb

@@ -26,8 +26,8 @@
     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)
+      allow(Gitlab::Git::Commit).to receive(:extract_signature)
+        .with(Gitlab::Git::Repository, commit_sha)
         .and_return(signature)
     end