Merge branch 'key_uniqueness' of /home/git/repositories/gitlab/gitlabhq

2 files changed, 15 insertions, 24 deletions

diff --git a/app/models/key.rb b/app/models/key.rb
index 11c01242b15..902a7c59116 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -21,11 +21,11 @@ class Key < ActiveRecord::Base
 
   attr_accessible :key, :title
 
-  before_validation :strip_white_space
+  before_validation :strip_white_space, :generate_fingerpint
 
   validates :title, presence: true, length: { within: 0..255 }
   validates :key, presence: true, length: { within: 0..5000 }, format: { with: /\A(ssh|ecdsa)-.*\Z/ }, uniqueness: true
-  validate :fingerprintable_key
+  validates :fingerprint, uniqueness: true, presence: { message: 'cannot be generated' }
 
   delegate :name, :email, to: :user, prefix: true
 
@@ -33,15 +33,6 @@ class Key < ActiveRecord::Base
     self.key = key.strip unless key.blank?
   end
 
-  def fingerprintable_key
-    return true unless key # Don't test if there is no key.
-
-    unless generate_fingerpint
-      errors.add(:key, "can't be fingerprinted")
-      false
-    end
-  end
-
   # projects that has this key
   def projects
     user.authorized_projects
@@ -54,26 +45,21 @@ class Key < ActiveRecord::Base
   private
 
   def generate_fingerpint
+    self.fingerprint = nil
+    return unless key.present?
+
     cmd_status = 0
     cmd_output = ''
-    file = Tempfile.new('gitlab_key_file')
-
-    begin
+    Tempfile.open('gitlab_key_file') do |file|
       file.puts key
       file.rewind
       cmd_output, cmd_status = popen("ssh-keygen -lf #{file.path}", '/tmp')
-    ensure
-      file.close
-      file.unlink # deletes the temp file
     end
 
     if cmd_status.zero?
       cmd_output.gsub /([\d\h]{2}:)+[\d\h]{2}/ do |match|
         self.fingerprint = match
       end
-      true
-    else
-      false
     end
   end
 end
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 6c06f5268b3..f4dd726331a 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -42,17 +42,22 @@ describe Key do
       build(:key, user: user).should be_valid
     end
 
-    it "does not accepts the key twice" do
+    it "does not accept the exact same key twice" do
       create(:key, user: user)
       build(:key, user: user).should_not be_valid
     end
+
+    it "does not accept a duplicate key with a different comment" do
+      create(:key, user: user)
+      duplicate = build(:key, user: user)
+      duplicate.key << ' extra comment'
+      duplicate.should_not be_valid
+    end
   end
 
   context "validate it is a fingerprintable key" do
-    let(:user) { create(:user) }
-
     it "accepts the fingerprintable key" do
-      build(:key, user: user).should be_valid
+      build(:key).should be_valid
     end
 
     it "rejects the unfingerprintable key (contains space in middle)" do