From 34810acd6c3d4dd27f43f6f07e47b4e06bb95f82 Mon Sep 17 00:00:00 2001
From: Alexis Reigel <mail@koffeinfrei.org>
Date: Thu, 15 Jun 2017 10:28:28 +0200
Subject: [PATCH] move signature cache read to Gpg::Commit
as we write the cache in the gpg commit class already the read should
also happen there.
This also removes all logic from the main commit class, which just
proxies the call to the Gpg::Commit now.
---
app/models/commit.rb | 5 --
lib/gitlab/gpg/commit.rb | 3 ++
spec/lib/gitlab/gpg/commit_spec.rb | 61 +++++++++++++++++-----
spec/models/commit_spec.rb | 82 ------------------------------
4 files changed, 52 insertions(+), 99 deletions(-)
diff --git a/app/models/commit.rb b/app/models/commit.rb
index ed8b9a79a7a..35593d53cbc 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -237,11 +237,6 @@ class Commit
def signature
return @signature if defined?(@signature)
- @signature = nil
-
- cached_signature = GpgSignature.find_by(commit_sha: sha)
- return cached_signature if cached_signature.present?
-
@signature = Gitlab::Gpg::Commit.new(self).signature
end
diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb
index d65a20f08f9..2b61caaebb5 100644
--- a/lib/gitlab/gpg/commit.rb
+++ b/lib/gitlab/gpg/commit.rb
@@ -16,6 +16,9 @@ module Gitlab
def signature
return unless has_signature?
+ cached_signature = GpgSignature.find_by(commit_sha: commit.sha)
+ return cached_signature if cached_signature.present?
+
Gitlab::Gpg.using_tmp_keychain do
# first we need to get the keyid from the signature to query the gpg
# key belonging to the keyid.
diff --git a/spec/lib/gitlab/gpg/commit_spec.rb b/spec/lib/gitlab/gpg/commit_spec.rb
index 2a583dc1bd5..539e6d4641f 100644
--- a/spec/lib/gitlab/gpg/commit_spec.rb
+++ b/spec/lib/gitlab/gpg/commit_spec.rb
@@ -11,19 +11,21 @@ RSpec.describe Gitlab::Gpg::Commit do
end
context 'known and verified public key' do
- it 'returns a valid signature' do
- gpg_key = create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
+ let!(:gpg_key) 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: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
allow(raw_commit).to receive :save!
- commit = create :commit,
- git_commit: raw_commit,
- project: project
+ create :commit, git_commit: raw_commit, project: project
+ end
+ it 'returns a valid signature' do
expect(described_class.new(commit).signature).to have_attributes(
commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
project: project,
@@ -32,22 +34,33 @@ RSpec.describe Gitlab::Gpg::Commit do
valid_signature: true
)
end
+
+ it 'returns the cached signature on second call' do
+ gpg_commit = described_class.new(commit)
+
+ expect(gpg_commit).to receive(:verified_signature).twice.and_call_original
+ gpg_commit.signature
+
+ # consecutive call
+ expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+ gpg_commit.signature
+ end
end
context 'known but unverified public key' do
- it 'returns an invalid signature' do
- gpg_key = create :gpg_key, key: GpgHelpers::User1.public_key
+ 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: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
allow(raw_commit).to receive :save!
- commit = create :commit,
- git_commit: raw_commit,
- project: project
+ create :commit, git_commit: raw_commit, project: project
+ end
+ it 'returns an invalid signature' do
expect(described_class.new(commit).signature).to have_attributes(
commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
project: project,
@@ -56,20 +69,33 @@ RSpec.describe Gitlab::Gpg::Commit do
valid_signature: false
)
end
+
+ it 'returns the cached signature on second call' do
+ gpg_commit = described_class.new(commit)
+
+ expect(gpg_commit).to receive(:verified_signature).and_call_original
+ gpg_commit.signature
+
+ # consecutive call
+ expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+ gpg_commit.signature
+ end
end
context 'unknown public key' do
- it 'returns an invalid signature', :gpg do
+ let!(:commit) do
raw_commit = double(:raw_commit, signature: [
GpgHelpers::User1.signed_commit_signature,
GpgHelpers::User1.signed_commit_base_data
], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
allow(raw_commit).to receive :save!
- commit = create :commit,
+ create :commit,
git_commit: raw_commit,
project: project
+ end
+ it 'returns an invalid signature' do
expect(described_class.new(commit).signature).to have_attributes(
commit_sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33',
project: project,
@@ -78,6 +104,17 @@ RSpec.describe Gitlab::Gpg::Commit do
valid_signature: false
)
end
+
+ it 'returns the cached signature on second call' do
+ gpg_commit = described_class.new(commit)
+
+ expect(gpg_commit).to receive(:verified_signature).and_call_original
+ gpg_commit.signature
+
+ # consecutive call
+ expect(gpg_commit).not_to receive(:verified_signature).and_call_original
+ gpg_commit.signature
+ end
end
end
end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 4370c78e6fd..528b211c9d6 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -414,86 +414,4 @@ eos
expect(described_class.valid_hash?('a' * 41)).to be false
end
end
-
- describe '#signature' do
- it 'returns nil if the commit is not signed' do
- expect(commit.signature).to be_nil
- end
-
- context 'signed commit', :gpg do
- context 'known public key' do
- it 'returns a valid signature' do
- create :gpg_key, key: GpgHelpers::User1.public_key
-
- raw_commit = double(:raw_commit, signature: [
- GpgHelpers::User1.signed_commit_signature,
- GpgHelpers::User1.signed_commit_base_data
- ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
- allow(raw_commit).to receive :save!
-
- commit = create :commit,
- git_commit: raw_commit,
- project: project
-
- expect(commit.signature.valid_signature?).to be_truthy
- end
-
- it 'returns the cached validation result on second call', :gpg do
- create :gpg_key, key: GpgHelpers::User1.public_key
-
- raw_commit = double(:raw_commit, signature: [
- GpgHelpers::User1.signed_commit_signature,
- GpgHelpers::User1.signed_commit_base_data
- ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
- allow(raw_commit).to receive :save!
-
- commit = create :commit,
- git_commit: raw_commit,
- project: project
-
- expect(Gitlab::Gpg::Commit).to receive(:new).and_call_original
- expect(commit.signature.valid_signature?).to be_truthy
-
- # second call returns the cache
- expect(Gitlab::Gpg::Commit).not_to receive(:new).and_call_original
- expect(commit.signature.valid_signature?).to be_truthy
- end
- end
-
- context 'unknown public key' do
- it 'returns an invalid signature if the public key is unknown', :gpg do
- raw_commit = double(:raw_commit, signature: [
- GpgHelpers::User1.signed_commit_signature,
- GpgHelpers::User1.signed_commit_base_data
- ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
- allow(raw_commit).to receive :save!
-
- commit = create :commit,
- git_commit: raw_commit,
- project: project
-
- expect(commit.signature.valid_signature?).to be_falsey
- end
-
- it 'returns the cached validation result on second call', :gpg do
- raw_commit = double(:raw_commit, signature: [
- GpgHelpers::User1.signed_commit_signature,
- GpgHelpers::User1.signed_commit_base_data
- ], sha: '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33')
- allow(raw_commit).to receive :save!
-
- commit = create :commit,
- git_commit: raw_commit,
- project: project
-
- expect(Gitlab::Gpg::Commit).to receive(:new).and_call_original
- expect(commit.signature.valid_signature?).to be_falsey
-
- # second call returns the cache
- expect(Gitlab::Gpg::Commit).not_to receive(:new).and_call_original
- expect(commit.signature.valid_signature?).to be_falsey
- end
- end
- end
- end
end
--
GitLab