From c50725fecfb776d56c95ef070940ca6c85786ecf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= <ruben@gitlab.com>
Date: Fri, 29 Sep 2017 17:55:36 -0500
Subject: [PATCH] Address feedback from last code review

---
 app/models/gpg_key.rb                           |  5 +++++
 app/models/gpg_key_subkey.rb                    |  3 +++
 app/models/gpg_signature.rb                     |  2 +-
 lib/gitlab/gpg.rb                               | 11 +++++------
 lib/gitlab/gpg/commit.rb                        |  8 +-------
 lib/gitlab/gpg/invalid_gpg_signature_updater.rb |  3 +--
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb
index 0ddf1245dd1..27513f4b03f 100644
--- a/app/models/gpg_key.rb
+++ b/app/models/gpg_key.rb
@@ -44,6 +44,7 @@ class GpgKey < ActiveRecord::Base
   def primary_keyid
     super&.upcase
   end
+  alias_method :keyid, :primary_keyid
 
   def fingerprint
     super&.upcase
@@ -53,6 +54,10 @@ class GpgKey < ActiveRecord::Base
     super(value&.strip)
   end
 
+  def keyids
+    [keyid].concat(subkeys.map(&:keyid))
+  end
+
   def user_infos
     @user_infos ||= Gitlab::Gpg.user_infos_from_key(key)
   end
diff --git a/app/models/gpg_key_subkey.rb b/app/models/gpg_key_subkey.rb
index b4f146e5647..1f3ec2a8f68 100644
--- a/app/models/gpg_key_subkey.rb
+++ b/app/models/gpg_key_subkey.rb
@@ -9,6 +9,9 @@ class GpgKeySubkey < ActiveRecord::Base
   validates :gpg_key_id, presence: true
   validates :fingerprint, :keyid, presence: true, uniqueness: true
 
+  delegate :key, :user, :user_infos, :verified?, :verified_user_infos,
+    :verified_and_belongs_to_email?, to: :gpg_key
+
   def keyid
     super&.upcase
   end
diff --git a/app/models/gpg_signature.rb b/app/models/gpg_signature.rb
index c7f75288407..d3cca19cea8 100644
--- a/app/models/gpg_signature.rb
+++ b/app/models/gpg_signature.rb
@@ -24,7 +24,7 @@ class GpgSignature < ActiveRecord::Base
   def gpg_key=(model)
     case model
     when GpgKey       then super
-    when GpgKeySubkey then write_attribute(:gpg_key_subkey_id, model.id)
+    when GpgKeySubkey then self.gpg_key_subkey = model
     end
   end
 
diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb
index 343bf54a6ae..413872d7e08 100644
--- a/lib/gitlab/gpg.rb
+++ b/lib/gitlab/gpg.rb
@@ -36,15 +36,14 @@ module Gitlab
 
     def subkeys_from_key(key)
       using_tmp_keychain do
-        fingerprints    = CurrentKeyChain.fingerprints_from_key(key)
-        raw_keys        = GPGME::Key.find(:public, fingerprints)
-        grouped_subkeys = Hash.new { |h, k| h[k] = [] }
+        fingerprints = CurrentKeyChain.fingerprints_from_key(key)
+        raw_keys     = GPGME::Key.find(:public, fingerprints)
 
-        raw_keys.each_with_object(grouped_subkeys).each do |raw_key, subkeys|
+        raw_keys.each_with_object({}) do |raw_key, grouped_subkeys|
           primary_subkey_id = raw_key.primary_subkey.keyid
 
-          raw_key.subkeys[1..-1].each do |subkey|
-            subkeys[primary_subkey_id] << { keyid: subkey.keyid, fingerprint: subkey.fingerprint }
+          grouped_subkeys[primary_subkey_id] = raw_key.subkeys[1..-1].map do |s|
+            { keyid: s.keyid, fingerprint: s.fingerprint }
           end
         end
       end
diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb
index 5cbc836314f..961c57ec0e6 100644
--- a/lib/gitlab/gpg/commit.rb
+++ b/lib/gitlab/gpg/commit.rb
@@ -74,7 +74,7 @@ module Gitlab
           commit_sha: @commit.sha,
           project: @commit.project,
           gpg_key: gpg_key,
-          gpg_key_primary_keyid: gpg_keyid(gpg_key) || verified_signature.fingerprint,
+          gpg_key_primary_keyid: gpg_key&.keyid || verified_signature.fingerprint,
           gpg_key_user_name: user_infos[:name],
           gpg_key_user_email: user_infos[:email],
           verification_status: verification_status
@@ -99,12 +99,6 @@ module Gitlab
         gpg_key&.verified_user_infos&.first || gpg_key&.user_infos&.first || {}
       end
 
-      def gpg_keyid(gpg_key)
-        return nil unless gpg_key
-
-        gpg_key.is_a?(GpgKey) ? gpg_key.primary_keyid : gpg_key.keyid
-      end
-
       def find_gpg_key(keyid)
         GpgKey.find_by(primary_keyid: keyid) || GpgKeySubkey.find_by(keyid: keyid)
       end
diff --git a/lib/gitlab/gpg/invalid_gpg_signature_updater.rb b/lib/gitlab/gpg/invalid_gpg_signature_updater.rb
index 9bad914848d..b7fb9dde2bc 100644
--- a/lib/gitlab/gpg/invalid_gpg_signature_updater.rb
+++ b/lib/gitlab/gpg/invalid_gpg_signature_updater.rb
@@ -3,14 +3,13 @@ module Gitlab
     class InvalidGpgSignatureUpdater
       def initialize(gpg_key)
         @gpg_key = gpg_key
-        @gpg_keyids = gpg_key.subkeys.map(&:keyid).push(gpg_key.primary_keyid)
       end
 
       def run
         GpgSignature
           .select(:id, :commit_sha, :project_id)
           .where('gpg_key_id IS NULL OR verification_status <> ?', GpgSignature.verification_statuses[:verified])
-          .where(gpg_key_primary_keyid: @gpg_keyids)
+          .where(gpg_key_primary_keyid: @gpg_key.keyids)
           .find_each { |sig| sig.gpg_commit.update_signature!(sig) }
       end
     end
-- 
GitLab