diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-08-15 14:22:55 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2017-08-16 19:57:50 +0300 |
commit | ba7251fefd92b0ecb6365cfe55510e24c5343ac6 (patch) | |
tree | d572877f0150efa654849b97896ef769aeff6c0b | |
parent | 4a2a6d521a260981482ee8e4931ebf06cb4f5b6a (diff) |
Only create commit GPG signature when necessary
-rw-r--r-- | app/models/commit.rb | 2 | ||||
-rw-r--r-- | app/models/gpg_signature.rb | 4 | ||||
-rw-r--r-- | app/services/git_push_service.rb | 15 | ||||
-rw-r--r-- | app/workers/create_gpg_signature_worker.rb | 8 | ||||
-rw-r--r-- | config/initializers/active_record_array_type_casting.rb | 20 | ||||
-rw-r--r-- | lib/gitlab/git/commit.rb | 19 | ||||
-rw-r--r-- | lib/gitlab/gpg/commit.rb | 38 | ||||
-rw-r--r-- | lib/gitlab/gpg/invalid_gpg_signature_updater.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/gpg/commit_spec.rb | 71 | ||||
-rw-r--r-- | spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb | 23 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 34 | ||||
-rw-r--r-- | spec/workers/create_gpg_signature_worker_spec.rb | 26 |
12 files changed, 160 insertions, 104 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index 638fddc5d3d..be9a56c190c 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -383,6 +383,6 @@ class Commit end def gpg_commit - @gpg_commit ||= Gitlab::Gpg::Commit.new(self) + @gpg_commit ||= Gitlab::Gpg::Commit.for_commit(self) end end diff --git a/app/models/gpg_signature.rb b/app/models/gpg_signature.rb index 1ac0e123ff1..50fb35c77ec 100644 --- a/app/models/gpg_signature.rb +++ b/app/models/gpg_signature.rb @@ -18,4 +18,8 @@ class GpgSignature < ActiveRecord::Base def commit project.commit(commit_sha) end + + def gpg_commit + Gitlab::Gpg::Commit.new(project, commit_sha) + end end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index ada2b64a3a6..e81a56672e2 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -90,8 +90,19 @@ class GitPushService < BaseService end def update_signatures - @push_commits.each do |commit| - CreateGpgSignatureWorker.perform_async(commit.sha, @project.id) + commit_shas = @push_commits.last(PROCESS_COMMIT_LIMIT).map(&:sha) + + return if commit_shas.empty? + + shas_with_cached_signatures = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha) + commit_shas -= shas_with_cached_signatures + + return if commit_shas.empty? + + commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas) + + commit_shas.each do |sha| + CreateGpgSignatureWorker.perform_async(sha, project.id) end end diff --git a/app/workers/create_gpg_signature_worker.rb b/app/workers/create_gpg_signature_worker.rb index 4f47717ff69..f34dff2d656 100644 --- a/app/workers/create_gpg_signature_worker.rb +++ b/app/workers/create_gpg_signature_worker.rb @@ -4,13 +4,9 @@ class CreateGpgSignatureWorker def perform(commit_sha, project_id) project = Project.find_by(id: project_id) - return unless project - commit = project.commit(commit_sha) - - return unless commit - - commit.signature + # This calculates and caches the signature in the database + Gitlab::Gpg::Commit.new(project, commit_sha).signature end end diff --git a/config/initializers/active_record_array_type_casting.rb b/config/initializers/active_record_array_type_casting.rb new file mode 100644 index 00000000000..d94d592add6 --- /dev/null +++ b/config/initializers/active_record_array_type_casting.rb @@ -0,0 +1,20 @@ +module ActiveRecord + class PredicateBuilder + class ArrayHandler + module TypeCasting + def call(attribute, value) + # This is necessary because by default ActiveRecord does not respect + # custom type definitions (like our `ShaAttribute`) when providing an + # array in `where`, like in `where(commit_sha: [sha1, sha2, sha3])`. + model = attribute.relation&.engine + type = model.user_provided_columns[attribute.name] if model + value = value.map { |value| type.type_cast_for_database(value) } if type + + super(attribute, value) + end + end + + prepend TypeCasting + end + end +end diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index fd4dfdb09a2..a499bbc6266 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -210,6 +210,16 @@ module Gitlab @rugged_sort_types.fetch(sort_type, Rugged::SORT_NONE) end + + def shas_with_signatures(repository, shas) + shas.select do |sha| + begin + Rugged::Commit.extract_signature(repository.rugged, sha) + rescue Rugged::OdbError + false + end + end + end end def initialize(repository, raw_commit, head = nil) @@ -335,15 +345,6 @@ module Gitlab parent_ids.map { |oid| self.class.find(@repository, oid) }.compact end - # Get the gpg signature of this commit. - # - # Ex. - # commit.signature(repo) - # - def signature(repo) - Rugged::Commit.extract_signature(repo.rugged, sha) - end - def stats Gitlab::Git::CommitStats.new(self) end diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb index 55428b85207..606c7576f70 100644 --- a/lib/gitlab/gpg/commit.rb +++ b/lib/gitlab/gpg/commit.rb @@ -1,12 +1,20 @@ module Gitlab module Gpg class Commit - attr_reader :commit + def self.for_commit(commit) + new(commit.project, commit.sha) + end - def initialize(commit) - @commit = commit + def initialize(project, sha) + @project = project + @sha = sha - @signature_text, @signed_text = commit.raw.signature(commit.project.repository) + @signature_text, @signed_text = + begin + Rugged::Commit.extract_signature(project.repository.rugged, sha) + rescue Rugged::OdbError + nil + end end def has_signature? @@ -16,18 +24,20 @@ module Gitlab def signature return unless has_signature? - cached_signature = GpgSignature.find_by(commit_sha: commit.sha) - return cached_signature if cached_signature.present? + return @signature if @signature - using_keychain do |gpg_key| - create_cached_signature!(gpg_key) - end + cached_signature = GpgSignature.find_by(commit_sha: @sha) + return @signature = cached_signature if cached_signature.present? + + @signature = create_cached_signature! end def update_signature!(cached_signature) using_keychain do |gpg_key| cached_signature.update_attributes!(attributes(gpg_key)) end + + @signature = cached_signature end private @@ -55,16 +65,18 @@ module Gitlab end end - def create_cached_signature!(gpg_key) - GpgSignature.create!(attributes(gpg_key)) + def create_cached_signature! + using_keychain do |gpg_key| + GpgSignature.create!(attributes(gpg_key)) + end end def attributes(gpg_key) user_infos = user_infos(gpg_key) { - commit_sha: commit.sha, - project: commit.project, + commit_sha: @sha, + project: @project, gpg_key: gpg_key, gpg_key_primary_keyid: gpg_key&.primary_keyid || verified_signature.fingerprint, gpg_key_user_name: user_infos[:name], diff --git a/lib/gitlab/gpg/invalid_gpg_signature_updater.rb b/lib/gitlab/gpg/invalid_gpg_signature_updater.rb index 3bb491120ba..a525ee7a9ee 100644 --- a/lib/gitlab/gpg/invalid_gpg_signature_updater.rb +++ b/lib/gitlab/gpg/invalid_gpg_signature_updater.rb @@ -10,9 +10,7 @@ module Gitlab .select(:id, :commit_sha, :project_id) .where('gpg_key_id IS NULL OR valid_signature = ?', false) .where(gpg_key_primary_keyid: @gpg_key.primary_keyid) - .find_each do |gpg_signature| - Gitlab::Gpg::Commit.new(gpg_signature.commit).update_signature!(gpg_signature) - end + .find_each { |sig| sig.gpg_commit.update_signature!(sig) } end end end 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 |