Commit b98f414a1b8dbadbccceac3a544c15bb456a952b
Exists in
master
and in
4 other branches
Merge branch 'key_uniqueness' of /home/git/repositories/gitlab/gitlabhq
Showing
2 changed files
with
15 additions
and
24 deletions
Show diff stats
app/models/key.rb
| ... | ... | @@ -21,11 +21,11 @@ class Key < ActiveRecord::Base |
| 21 | 21 | |
| 22 | 22 | attr_accessible :key, :title |
| 23 | 23 | |
| 24 | - before_validation :strip_white_space | |
| 24 | + before_validation :strip_white_space, :generate_fingerpint | |
| 25 | 25 | |
| 26 | 26 | validates :title, presence: true, length: { within: 0..255 } |
| 27 | 27 | validates :key, presence: true, length: { within: 0..5000 }, format: { with: /\A(ssh|ecdsa)-.*\Z/ }, uniqueness: true |
| 28 | - validate :fingerprintable_key | |
| 28 | + validates :fingerprint, uniqueness: true, presence: { message: 'cannot be generated' } | |
| 29 | 29 | |
| 30 | 30 | delegate :name, :email, to: :user, prefix: true |
| 31 | 31 | |
| ... | ... | @@ -33,15 +33,6 @@ class Key < ActiveRecord::Base |
| 33 | 33 | self.key = key.strip unless key.blank? |
| 34 | 34 | end |
| 35 | 35 | |
| 36 | - def fingerprintable_key | |
| 37 | - return true unless key # Don't test if there is no key. | |
| 38 | - | |
| 39 | - unless generate_fingerpint | |
| 40 | - errors.add(:key, "can't be fingerprinted") | |
| 41 | - false | |
| 42 | - end | |
| 43 | - end | |
| 44 | - | |
| 45 | 36 | # projects that has this key |
| 46 | 37 | def projects |
| 47 | 38 | user.authorized_projects |
| ... | ... | @@ -54,26 +45,21 @@ class Key < ActiveRecord::Base |
| 54 | 45 | private |
| 55 | 46 | |
| 56 | 47 | def generate_fingerpint |
| 48 | + self.fingerprint = nil | |
| 49 | + return unless key.present? | |
| 50 | + | |
| 57 | 51 | cmd_status = 0 |
| 58 | 52 | cmd_output = '' |
| 59 | - file = Tempfile.new('gitlab_key_file') | |
| 60 | - | |
| 61 | - begin | |
| 53 | + Tempfile.open('gitlab_key_file') do |file| | |
| 62 | 54 | file.puts key |
| 63 | 55 | file.rewind |
| 64 | 56 | cmd_output, cmd_status = popen("ssh-keygen -lf #{file.path}", '/tmp') |
| 65 | - ensure | |
| 66 | - file.close | |
| 67 | - file.unlink # deletes the temp file | |
| 68 | 57 | end |
| 69 | 58 | |
| 70 | 59 | if cmd_status.zero? |
| 71 | 60 | cmd_output.gsub /([\d\h]{2}:)+[\d\h]{2}/ do |match| |
| 72 | 61 | self.fingerprint = match |
| 73 | 62 | end |
| 74 | - true | |
| 75 | - else | |
| 76 | - false | |
| 77 | 63 | end |
| 78 | 64 | end |
| 79 | 65 | end | ... | ... |
spec/models/key_spec.rb
| ... | ... | @@ -42,17 +42,22 @@ describe Key do |
| 42 | 42 | build(:key, user: user).should be_valid |
| 43 | 43 | end |
| 44 | 44 | |
| 45 | - it "does not accepts the key twice" do | |
| 45 | + it "does not accept the exact same key twice" do | |
| 46 | 46 | create(:key, user: user) |
| 47 | 47 | build(:key, user: user).should_not be_valid |
| 48 | 48 | end |
| 49 | + | |
| 50 | + it "does not accept a duplicate key with a different comment" do | |
| 51 | + create(:key, user: user) | |
| 52 | + duplicate = build(:key, user: user) | |
| 53 | + duplicate.key << ' extra comment' | |
| 54 | + duplicate.should_not be_valid | |
| 55 | + end | |
| 49 | 56 | end |
| 50 | 57 | |
| 51 | 58 | context "validate it is a fingerprintable key" do |
| 52 | - let(:user) { create(:user) } | |
| 53 | - | |
| 54 | 59 | it "accepts the fingerprintable key" do |
| 55 | - build(:key, user: user).should be_valid | |
| 60 | + build(:key).should be_valid | |
| 56 | 61 | end |
| 57 | 62 | |
| 58 | 63 | it "rejects the unfingerprintable key (contains space in middle)" do | ... | ... |