Refactor signatures into CommitSignature module

Commit signatures are currently top-level models and have some
duplicated code. This change moves the signatures into a CommitSignature
module, and moves shared code into a concern.

.rubocop_todo/performance/active_record_subtransaction_methods.yml

@@ -10,7 +10,6 @@ Performance/ActiveRecordSubtransactionMethods:
   - app/models/design_management/design_collection.rb
   - app/models/error_tracking/error.rb
   - app/models/external_pull_request.rb
-  - app/models/gpg_signature.rb
   - app/models/merge_request.rb
   - app/models/plan.rb
   - app/models/project.rb
@@ -18,6 +17,7 @@ Performance/ActiveRecordSubtransactionMethods:
   - app/models/x509_certificate.rb
   - app/models/x509_commit_signature.rb
   - app/models/x509_issuer.rb
+  - app/models/concerns/commit_signature.rb
   - app/services/bulk_imports/relation_export_service.rb
   - app/services/ci/update_build_state_service.rb
   - app/services/event_create_service.rb

app/helpers/x509_helper.rb

@@ -18,6 +18,6 @@ module X509Helper
   end
 
   def x509_signature?(sig)
-    sig.is_a?(X509CommitSignature) || sig.is_a?(Gitlab::X509::Signature)
+    sig.is_a?(CommitSignatures::X509CommitSignature) || sig.is_a?(Gitlab::X509::Signature)
   end
 end

app/models/commit_signatures/gpg_signature.rb

+# frozen_string_literal: true
+module CommitSignatures
+  class GpgSignature < ApplicationRecord
+    include CommitSignature
+
+    sha_attribute :gpg_key_primary_keyid
+
+    belongs_to :gpg_key
+    belongs_to :gpg_key_subkey
+
+    validates :gpg_key_primary_keyid, presence: true
+
+    def self.with_key_and_subkeys(gpg_key)
+      subkey_ids = gpg_key.subkeys.pluck(:id)
+
+      where(
+        arel_table[:gpg_key_id].eq(gpg_key.id).or(
+          arel_table[:gpg_key_subkey_id].in(subkey_ids)
+        )
+      )
+    end
+
+    def gpg_key=(model)
+      case model
+      when GpgKey
+        super
+      when GpgKeySubkey
+        self.gpg_key_subkey = model
+      when NilClass
+        super
+        self.gpg_key_subkey = nil
+      end
+    end
+
+    def gpg_key
+      if gpg_key_id
+        super
+      elsif gpg_key_subkey_id
+        gpg_key_subkey
+      end
+    end
+
+    def gpg_key_primary_keyid
+      super&.upcase
+    end
+
+    def gpg_commit
+      return unless commit
+
+      Gitlab::Gpg::Commit.new(commit)
+    end
+  end
+end

app/models/commit_signatures/x509_commit_signature.rb

+# frozen_string_literal: true
+module CommitSignatures
+  class X509CommitSignature < ApplicationRecord
+    include CommitSignature
+
+    belongs_to :x509_certificate, class_name: 'X509Certificate', foreign_key: 'x509_certificate_id', optional: false
+
+    validates :x509_certificate_id, presence: true
+
+    def x509_commit
+      return unless commit
+
+      Gitlab::X509::Commit.new(commit)
+    end
+  end
+end

app/models/concerns/commit_signature.rb

+# frozen_string_literal: true
+module CommitSignature
+  extend ActiveSupport::Concern
+
+  included do
+    include ShaAttribute
+
+    sha_attribute :commit_sha
+
+    enum verification_status: {
+      unverified: 0,
+      verified: 1,
+      same_user_different_email: 2,
+      other_user: 3,
+      unverified_key: 4,
+      unknown_key: 5,
+      multiple_signatures: 6
+    }
+
+    belongs_to :project, class_name: 'Project', foreign_key: 'project_id', optional: false
+
+    validates :commit_sha, presence: true
+    validates :project_id, presence: true
+
+    scope :by_commit_sha, ->(shas) { where(commit_sha: shas) }
+  end
+
+  class_methods do
+    def safe_create!(attributes)
+      create_with(attributes)
+        .safe_find_or_create_by!(commit_sha: attributes[:commit_sha])
+    end
+
+    # Find commits that are lacking a signature in the database at present
+    def unsigned_commit_shas(commit_shas)
+      return [] if commit_shas.empty?
+
+      signed = by_commit_sha(commit_shas).pluck(:commit_sha)
+      commit_shas - signed
+    end
+  end
+
+  def commit
+    project.commit(commit_sha)
+  end
+
+  def user
+    commit.committer
+  end
+end

app/models/gpg_key.rb

@@ -92,13 +92,13 @@ class GpgKey < ApplicationRecord
   end
 
   def revoke
-    GpgSignature
+    CommitSignatures::GpgSignature
       .with_key_and_subkeys(self)
-      .where.not(verification_status: GpgSignature.verification_statuses[:unknown_key])
+      .where.not(verification_status: CommitSignatures::GpgSignature.verification_statuses[:unknown_key])
       .update_all(
         gpg_key_id: nil,
         gpg_key_subkey_id: nil,
-        verification_status: GpgSignature.verification_statuses[:unknown_key],
+        verification_status: CommitSignatures::GpgSignature.verification_statuses[:unknown_key],
         updated_at: Time.zone.now
       )
 

app/models/gpg_signature.rb

-# frozen_string_literal: true
-
-class GpgSignature < ApplicationRecord
-  include ShaAttribute
-
-  sha_attribute :commit_sha
-  sha_attribute :gpg_key_primary_keyid
-
-  enum verification_status: {
-    unverified: 0,
-    verified: 1,
-    same_user_different_email: 2,
-    other_user: 3,
-    unverified_key: 4,
-    unknown_key: 5,
-    multiple_signatures: 6
-  }
-
-  belongs_to :project
-  belongs_to :gpg_key
-  belongs_to :gpg_key_subkey
-
-  validates :commit_sha, presence: true
-  validates :project_id, presence: true
-  validates :gpg_key_primary_keyid, presence: true
-
-  scope :by_commit_sha, ->(shas) { where(commit_sha: shas) }
-
-  def self.with_key_and_subkeys(gpg_key)
-    subkey_ids = gpg_key.subkeys.pluck(:id)
-
-    where(
-      arel_table[:gpg_key_id].eq(gpg_key.id).or(
-        arel_table[:gpg_key_subkey_id].in(subkey_ids)
-      )
-    )
-  end
-
-  def self.safe_create!(attributes)
-    create_with(attributes)
-      .safe_find_or_create_by!(commit_sha: attributes[:commit_sha])
-  end
-
-  # Find commits that are lacking a signature in the database at present
-  def self.unsigned_commit_shas(commit_shas)
-    return [] if commit_shas.empty?
-
-    signed = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha)
-
-    commit_shas - signed
-  end
-
-  def gpg_key=(model)
-    case model
-    when GpgKey
-      super
-    when GpgKeySubkey
-      self.gpg_key_subkey = model
-    when NilClass
-      super
-      self.gpg_key_subkey = nil
-    end
-  end
-
-  def gpg_key
-    if gpg_key_id
-      super
-    elsif gpg_key_subkey_id
-      gpg_key_subkey
-    end
-  end
-
-  def gpg_key_primary_keyid
-    super&.upcase
-  end
-
-  def commit
-    project.commit(commit_sha)
-  end
-
-  def gpg_commit
-    return unless commit
-
-    Gitlab::Gpg::Commit.new(commit)
-  end
-end

app/models/x509_certificate.rb

@@ -13,7 +13,7 @@ class X509Certificate < ApplicationRecord
 
   belongs_to :x509_issuer, class_name: 'X509Issuer', foreign_key: 'x509_issuer_id', optional: false
 
-  has_many :x509_commit_signatures, inverse_of: 'x509_certificate'
+  has_many :x509_commit_signatures, class_name: 'CommitSignatures::X509CommitSignature', inverse_of: 'x509_certificate'
 
   # rfc 5280 - 4.2.1.2  Subject Key Identifier
   validates :subject_key_identifier, presence: true, format: { with: /\A(\h{2}:){19}\h{2}\z/ }

app/models/x509_commit_signature.rb

-# frozen_string_literal: true
-
-class X509CommitSignature < ApplicationRecord
-  include ShaAttribute
-
-  sha_attribute :commit_sha
-
-  enum verification_status: {
-    unverified: 0,
-    verified: 1
-  }
-
-  belongs_to :project, class_name: 'Project', foreign_key: 'project_id', optional: false
-  belongs_to :x509_certificate, class_name: 'X509Certificate', foreign_key: 'x509_certificate_id', optional: false
-
-  validates :commit_sha, presence: true
-  validates :project_id, presence: true
-  validates :x509_certificate_id, presence: true
-
-  scope :by_commit_sha, ->(shas) { where(commit_sha: shas) }
-
-  def self.safe_create!(attributes)
-    create_with(attributes)
-      .safe_find_or_create_by!(commit_sha: attributes[:commit_sha])
-  end
-
-  # Find commits that are lacking a signature in the database at present
-  def self.unsigned_commit_shas(commit_shas)
-    return [] if commit_shas.empty?
-
-    signed = by_commit_sha(commit_shas).pluck(:commit_sha)
-    commit_shas - signed
-  end
-
-  def commit
-    project.commit(commit_sha)
-  end
-
-  def x509_commit
-    return unless commit
-
-    Gitlab::X509::Commit.new(commit)
-  end
-
-  def user
-    commit.committer
-  end
-end

app/services/git/branch_hooks_service.rb

@@ -157,11 +157,11 @@ module Git
     end
 
     def unsigned_x509_shas(commits)
-      X509CommitSignature.unsigned_commit_shas(commits.map(&:sha))
+      CommitSignatures::X509CommitSignature.unsigned_commit_shas(commits.map(&:sha))
     end
 
     def unsigned_gpg_shas(commits)
-      GpgSignature.unsigned_commit_shas(commits.map(&:sha))
+      CommitSignatures::GpgSignature.unsigned_commit_shas(commits.map(&:sha))
     end
 
     def enqueue_update_signatures

doc/user/project/repository/x509_signed_commits/index.md

@@ -290,7 +290,7 @@ To investigate why a commit shows as `Unverified`:
 1. The verification status is stored in the database. To display the database record:
 
    ```ruby
-   pp X509CommitSignature.by_commit_sha(commit.sha);nil
+   pp CommitSignatures::X509CommitSignature.by_commit_sha(commit.sha);nil
    ```
 
    If all the previous checks returned the correct values:

lib/api/entities/commit_signature.rb

@@ -6,9 +6,9 @@ module API
       expose :signature_type
 
       expose :signature, merge: true do |commit, options|
-        if commit.signature.is_a?(GpgSignature) || commit.raw_commit_from_rugged?
+        if commit.signature.is_a?(::CommitSignatures::GpgSignature) || commit.raw_commit_from_rugged?
           ::API::Entities::GpgCommitSignature.represent commit_signature(commit), options
-        elsif commit.signature.is_a?(X509CommitSignature)
+        elsif commit.signature.is_a?(::CommitSignatures::X509CommitSignature)
           ::API::Entities::X509Signature.represent commit.signature, options
         end
       end

lib/gitlab/gpg/commit.rb

@@ -25,7 +25,7 @@ module Gitlab
 
       def lazy_signature
         BatchLoader.for(@commit.sha).batch do |shas, loader|
-          GpgSignature.by_commit_sha(shas).each do |signature|
+          CommitSignatures::GpgSignature.by_commit_sha(shas).each do |signature|
             loader.call(signature.commit_sha, signature)
           end
         end
@@ -62,9 +62,9 @@ module Gitlab
       def create_cached_signature!
         using_keychain do |gpg_key|
           attributes = attributes(gpg_key)
-          break GpgSignature.new(attributes) if Gitlab::Database.read_only?
+          break CommitSignatures::GpgSignature.new(attributes) if Gitlab::Database.read_only?
 
-          GpgSignature.safe_create!(attributes)
+          CommitSignatures::GpgSignature.safe_create!(attributes)
         end
       end
 

lib/gitlab/gpg/invalid_gpg_signature_updater.rb

@@ -9,9 +9,9 @@ module Gitlab
 
       # rubocop: disable CodeReuse/ActiveRecord
       def run
-        GpgSignature
+        CommitSignatures::GpgSignature
           .select(:id, :commit_sha, :project_id)
-          .where('gpg_key_id IS NULL OR verification_status <> ?', GpgSignature.verification_statuses[:verified])
+          .where('gpg_key_id IS NULL OR verification_status <> ?', CommitSignatures::GpgSignature.verification_statuses[:verified])
           .where(gpg_key_primary_keyid: @gpg_key.keyids)
           .find_each { |sig| sig.gpg_commit&.update_signature!(sig) }
       end

lib/gitlab/x509/commit.rb

@@ -25,7 +25,7 @@ module Gitlab
 
       def lazy_signature
         BatchLoader.for(@commit.sha).batch do |shas, loader|
-          X509CommitSignature.by_commit_sha(shas).each do |signature|
+          CommitSignatures::X509CommitSignature.by_commit_sha(shas).each do |signature|
             loader.call(signature.commit_sha, signature)
           end
         end
@@ -49,9 +49,9 @@ module Gitlab
       def create_cached_signature!
         return if attributes.nil?
 
-        return X509CommitSignature.new(attributes) if Gitlab::Database.read_only?
+        return CommitSignatures::X509CommitSignature.new(attributes) if Gitlab::Database.read_only?
 
-        X509CommitSignature.safe_create!(attributes)
+        CommitSignatures::X509CommitSignature.safe_create!(attributes)
       end
     end
   end

lib/tasks/gitlab/x509/update.rake

@@ -12,14 +12,14 @@ namespace :gitlab do
     def update_certificates
       logger = Logger.new($stdout)
 
-      unless X509CommitSignature.exists?
+      unless CommitSignatures::X509CommitSignature.exists?
         logger.info("Unable to find any x509 commit signatures. Exiting.")
         return
       end
 
       logger.info("Start to update x509 commit signatures")
 
-      X509CommitSignature.find_each do |sig|
+      CommitSignatures::X509CommitSignature.find_each do |sig|
         sig.x509_commit&.update_signature!(sig)
       end
 

spec/factories/gpg_signature.rb → spec/factories/commit_signature/gpg_signature.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :gpg_signature do
+  factory :gpg_signature, class: 'CommitSignatures::GpgSignature' do
     commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
     project
     gpg_key

spec/factories/x509_commit_signature.rb → spec/factories/commit_signature/x509_commit_signature.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :x509_commit_signature do
+  factory :x509_commit_signature, class: 'CommitSignatures::X509CommitSignature' do
     commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
     project
     x509_certificate

spec/models/gpg_signature_spec.rb → spec/models/commit_signatures/gpg_signature_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe GpgSignature do
+RSpec.describe CommitSignatures::GpgSignature do
   let(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
   let!(:project) { create(:project, :repository, path: 'sample-project') }
   let!(:commit) { create(:commit, project: project, sha: commit_sha) }
@@ -13,7 +13,7 @@ RSpec.describe GpgSignature do
   it_behaves_like 'having unique enum values'
 
   describe 'associations' do
-    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:project).required }
     it { is_expected.to belong_to(:gpg_key) }
     it { is_expected.to belong_to(:gpg_key_subkey) }
   end

spec/models/x509_commit_signature_spec.rb → spec/models/commit_signatures/x509_commit_signature_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe X509CommitSignature do
+RSpec.describe CommitSignatures::X509CommitSignature do
   let(:commit_sha) { '189a6c924013fc3fe40d6f1ec1dc20214183bc97' }
   let(:project) { create(:project, :public, :repository) }
   let!(:commit) { create(:commit, project: project, sha: commit_sha) }

spec/workers/create_commit_signature_worker_spec.rb

@@ -143,7 +143,7 @@ RSpec.describe CreateCommitSignatureWorker do
       let(:type) { :X509 }
 
       it 'performs a single query for commit signatures' do
-        expect(X509CommitSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
+        expect(CommitSignatures::X509CommitSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
 
         subject
       end
@@ -153,7 +153,7 @@ RSpec.describe CreateCommitSignatureWorker do
       let(:type) { :PGP }
 
       it 'performs a single query for commit signatures' do
-        expect(GpgSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
+        expect(CommitSignatures::GpgSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
 
         subject
       end