remove gpg from keychain when user's email changes

6 files changed, 118 insertions, 40 deletions

diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb
index 83a303ae953..8332ba3ee6e 100644
--- a/app/models/gpg_key.rb
+++ b/app/models/gpg_key.rb
@@ -21,9 +21,9 @@ class GpgKey < ActiveRecord::Base
     unless: -> { errors.has_key?(:key) }
 
   before_validation :extract_fingerprint
-  after_create :add_to_keychain
+  after_create :synchronize_keychain
   after_create :notify_user
-  after_destroy :remove_from_keychain
+  after_destroy :synchronize_keychain
 
   def key=(value)
     value.strip! unless value.blank?
@@ -31,21 +31,32 @@ class GpgKey < ActiveRecord::Base
   end
 
   def emails
-    Gitlab::Gpg::CurrentKeyChain.emails(fingerprint)
+    @emails ||= Gitlab::Gpg.emails_from_key(key)
   end
 
-  def emails_with_verified_status
-    emails_in_key_chain = emails
-    emails_from_key = Gitlab::Gpg.emails_from_key(key)
+  def emails_in_keychain
+    @emails_in_keychain ||= Gitlab::Gpg::CurrentKeyChain.emails(fingerprint)
+  end
 
-    emails_from_key.map do |email|
+  def emails_with_verified_status
+    emails.map do |email|
       [
         email,
-        email == user.email && emails_in_key_chain.include?(email)
+        email == user.email && emails_in_keychain.include?(email)
       ]
     end
   end
 
+  def synchronize_keychain
+    if emails.include?(user.email)
+      add_to_keychain
+    else
+      remove_from_keychain
+    end
+
+    @emails_in_keychain = nil
+  end
+
   private
 
   def extract_fingerprint
@@ -55,10 +66,6 @@ class GpgKey < ActiveRecord::Base
   end
 
   def add_to_keychain
-    emails_from_key = Gitlab::Gpg.emails_from_key(key)
-
-    return unless emails_from_key.include?(user.email)
-
     Gitlab::Gpg::CurrentKeyChain.add(key)
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 5aebd36cf8a..42c83e3d206 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -155,6 +155,7 @@ class User < ActiveRecord::Base
   before_validation :set_public_email, if: :public_email_changed?
 
   after_update :update_emails_with_primary_email, if: :email_changed?
+  after_update :synchronize_gpg_keys, if: :email_changed?
   before_save :ensure_authentication_token, :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: :external_changed?
   after_save :ensure_namespace_correct
@@ -1157,4 +1158,10 @@ class User < ActiveRecord::Base
   ensure
     Gitlab::ExclusiveLease.cancel(lease_key, uuid)
   end
+
+  def synchronize_gpg_keys
+    gpg_keys.each do |gpg_key|
+      gpg_key.synchronize_keychain
+    end
+  end
 end
diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb
index f478f1ae5d8..ee0467ae264 100644
--- a/lib/gitlab/gpg.rb
+++ b/lib/gitlab/gpg.rb
@@ -10,7 +10,9 @@ module Gitlab
       end
 
       def remove(fingerprint)
-        GPGME::Key.get(fingerprint).delete!
+        # `#get` raises an EOFError if the keychain is empty, which is why we
+        # use the friendlier `#find`
+        GPGME::Key.find(:public, fingerprint).each(&:delete!)
       end
 
       def emails(fingerprint)
diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb
index c303f29a832..79952eda2ff 100644
--- a/spec/features/commits_spec.rb
+++ b/spec/features/commits_spec.rb
@@ -206,7 +206,8 @@ describe 'Commits' do
   end
 
   describe 'GPG signed commits' do
-    let(:user) { create(:user) }
+    let!(:user) { create :user, email: GpgHelpers::User1.emails.first }
+    let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key, user: user }
 
     before do
       project.team << [user, :master]
@@ -214,8 +215,6 @@ describe 'Commits' do
     end
 
     it 'shows the signed status', :gpg do
-      GPGME::Key.import(GpgHelpers::User1.public_key)
-
       # FIXME: add this to the test repository directly
       remote_path = project.repository.path_to_repo
       Dir.mktmpdir do |dir|
@@ -233,6 +232,17 @@ describe 'Commits' do
         expect(page).to have_content 'Unverified'
         expect(page).to have_content 'Verified'
       end
+
+      # user changes his email which makes the gpg key unverified
+      user.skip_reconfirmation!
+      user.update_attributes!(email: 'bette.cartwright@example.org')
+
+      visit namespace_project_commits_path(project.namespace, project, :master)
+
+      within '#commits-list' do
+        expect(page).to have_content 'Unverified'
+        expect(page).not_to have_content 'Verified'
+      end
     end
   end
 end
diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb
index 4292892da4f..18746ad9d88 100644
--- a/spec/models/gpg_key_spec.rb
+++ b/spec/models/gpg_key_spec.rb
@@ -22,33 +22,16 @@ describe GpgKey do
       end
     end
 
-    describe 'add_to_keychain' do
-      context "user's email matches one of the key's emails" do
-        it 'calls .add after create' do
-          expect(Gitlab::Gpg::CurrentKeyChain).to receive(:add).with(GpgHelpers::User2.public_key)
-          user = create :user, email: GpgHelpers::User2.emails.first
-          create :gpg_key, user: user, key: GpgHelpers::User2.public_key
-        end
+    describe 'synchronize_keychain' do
+      it 'calls #synchronize_keychain after create' do
+        gpg_key = build :gpg_key
+        expect(gpg_key).to receive(:synchronize_keychain)
+        gpg_key.save!
       end
 
-      context "user's email does not match one of the key's emails" do
-        it 'does not call .add after create' do
-          expect(Gitlab::Gpg::CurrentKeyChain).not_to receive(:add)
-          user = create :user
-          create :gpg_key, user: user, key: GpgHelpers::User2.public_key
-        end
-      end
-    end
-
-    describe 'remove_from_keychain' do
-      it 'calls remove_from_keychain after destroy' do
-        allow(Gitlab::Gpg::CurrentKeyChain).to receive :add
+      it 'calls #remove_from_keychain after destroy' do
         gpg_key = create :gpg_key
-
-        expect(
-          Gitlab::Gpg::CurrentKeyChain
-        ).to receive(:remove).with(GpgHelpers::User1.fingerprint)
-
+        expect(gpg_key).to receive(:synchronize_keychain)
         gpg_key.destroy!
       end
     end
@@ -76,6 +59,15 @@ describe GpgKey do
     end
   end
 
+  describe '#emails_in_keychain', :gpg do
+    it 'returns the emails from the keychain' do
+      user = create :user, email: GpgHelpers::User1.emails.first
+      gpg_key = create :gpg_key, key: GpgHelpers::User1.public_key, user: user
+
+      expect(gpg_key.emails_in_keychain).to eq GpgHelpers::User1.emails
+    end
+  end
+
   describe '#emails_with_verified_status', :gpg do
     context 'key is in the keychain' do
       it 'email is verified if the user has the matching email' do
@@ -104,6 +96,46 @@ describe GpgKey do
     end
   end
 
+  describe '#synchronize_keychain', :gpg do
+    context "user's email matches one of the key's emails" do
+      it 'adds the key to the keychain' do
+        user = create :user, email: GpgHelpers::User1.emails.first
+        gpg_key = create :gpg_key, user: user
+
+        expect(gpg_key).to receive(:add_to_keychain)
+
+        gpg_key.synchronize_keychain
+      end
+    end
+
+    context "user's email does not match one of the key's emails" do
+      it 'does not add the key to the keychain' do
+        user = create :user, email: 'stepanie@cole.us'
+        gpg_key = create :gpg_key, user: user
+
+        expect(gpg_key).to receive(:remove_from_keychain)
+
+        gpg_key.synchronize_keychain
+      end
+    end
+  end
+
+  describe '#add_to_keychain', :gpg do
+    it 'calls .add_to_keychain' do
+      expect(Gitlab::Gpg::CurrentKeyChain).to receive(:add).with(GpgHelpers::User2.public_key)
+      gpg_key = create :gpg_key, key: GpgHelpers::User2.public_key
+      gpg_key.send(:add_to_keychain)
+    end
+  end
+
+  describe '#remove_from_keychain', :gpg do
+    it 'calls .remove_from_keychain' do
+      allow(Gitlab::Gpg::CurrentKeyChain).to receive(:remove).with(GpgHelpers::User2.fingerprint)
+      gpg_key = create :gpg_key, key: GpgHelpers::User2.public_key
+      gpg_key.send(:remove_from_keychain)
+    end
+  end
+
   describe 'notification' do
     include EmailHelpers
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 20bdb7e37da..60979fd6c06 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1956,4 +1956,24 @@ describe User, models: true do
       expect(user.allow_password_authentication?).to be_falsey
     end
   end
+
+  context 'callbacks' do
+    context '.synchronize_gpg_keys' do
+      let(:user) do
+        create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
+          user.skip_reconfirmation!
+        end
+      end
+
+      it 'does nothing when the name is updated' do
+        expect(user).not_to receive(:synchronize_gpg_keys)
+        user.update_attributes!(name: 'Bette')
+      end
+
+      it 'synchronizes the gpg keys when the email is updated' do
+        expect(user).to receive(:synchronize_gpg_keys)
+        user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
+      end
+    end
+  end
 end