diff options
author | Rubén Dávila <ruben@gitlab.com> | 2018-02-12 20:39:47 +0300 |
---|---|---|
committer | Rubén Dávila <ruben@gitlab.com> | 2018-02-12 20:45:57 +0300 |
commit | 1fd8f0ef7a95d682a92fe1f82d0117058e39498b (patch) | |
tree | d640a75b58185067d7f30e0c2821ce8fc654b153 | |
parent | 2347e31671333cb1957bca91aae2ff81b641751d (diff) |
Revert "Merge branch 'rd-40552-gitlab-should-check-if-keys-are-valid-before-saving' into 'master'"revert-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key
This reverts commit a58f8c32c62bcf5824d1fe1d0de53e9bda974d65, reversing
changes made to cd5d75c362cdf06efb8174eddfbd0f4b65687dec.
-rw-r--r-- | app/models/key.rb | 7 | ||||
-rw-r--r-- | changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/ssh_public_key.rb | 28 | ||||
-rw-r--r-- | spec/factories/keys.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/ssh_public_key_spec.rb | 35 | ||||
-rw-r--r-- | spec/models/key_spec.rb | 51 |
6 files changed, 17 insertions, 113 deletions
diff --git a/app/models/key.rb b/app/models/key.rb index ae5769c0627..7406c98c99e 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -33,8 +33,9 @@ class Key < ActiveRecord::Base after_destroy :refresh_user_cache def key=(value) - write_attribute(:key, value.present? ? Gitlab::SSHPublicKey.sanitize(value) : nil) - + value&.delete!("\n\r") + value.strip! unless value.blank? + write_attribute(:key, value) @public_key = nil end @@ -96,7 +97,7 @@ class Key < ActiveRecord::Base def generate_fingerprint self.fingerprint = nil - return unless public_key.valid? + return unless self.key.present? self.fingerprint = public_key.fingerprint end diff --git a/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml b/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml deleted file mode 100644 index 9e4811ca308..00000000000 --- a/changelogs/unreleased/40552-sanitize-extra-blank-spaces-used-when-uploading-a-ssh-key.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Sanitize extra blank spaces used when uploading a SSH key -merge_request: 40552 -author: -type: fixed diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb index 545e7c74f7e..89ca1298120 100644 --- a/lib/gitlab/ssh_public_key.rb +++ b/lib/gitlab/ssh_public_key.rb @@ -21,22 +21,6 @@ module Gitlab technology(name)&.supported_sizes end - def self.sanitize(key_content) - ssh_type, *parts = key_content.strip.split - - return key_content if parts.empty? - - parts.each_with_object("#{ssh_type} ").with_index do |(part, content), index| - content << part - - if Gitlab::SSHPublicKey.new(content).valid? - break [content, parts[index + 1]].compact.join(' ') # Add the comment part if present - elsif parts.size == index + 1 # return original content if we've reached the last element - break key_content - end - end - end - attr_reader :key_text, :key # Unqualified MD5 fingerprint for compatibility @@ -53,23 +37,23 @@ module Gitlab end def valid? - key.present? && bits && technology.supported_sizes.include?(bits) + key.present? end def type - technology.name if key.present? + technology.name if valid? end def bits - return if key.blank? + return unless valid? case type when :rsa - key.n&.num_bits + key.n.num_bits when :dsa - key.p&.num_bits + key.p.num_bits when :ecdsa - key.group.order&.num_bits + key.group.order.num_bits when :ed25519 256 else diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index 23a98a899f1..f0c43f3d6f5 100644 --- a/spec/factories/keys.rb +++ b/spec/factories/keys.rb @@ -5,10 +5,6 @@ FactoryBot.define do title key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate + ' dummy@gitlab.com' } - factory :key_without_comment do - key { Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate } - end - factory :deploy_key, class: 'DeployKey' factory :personal_key do diff --git a/spec/lib/gitlab/ssh_public_key_spec.rb b/spec/lib/gitlab/ssh_public_key_spec.rb index c15e29774b6..93d538141ce 100644 --- a/spec/lib/gitlab/ssh_public_key_spec.rb +++ b/spec/lib/gitlab/ssh_public_key_spec.rb @@ -37,41 +37,6 @@ describe Gitlab::SSHPublicKey, lib: true do end end - describe '.sanitize(key_content)' do - let(:content) { build(:key).key } - - context 'when key has blank space characters' do - it 'removes the extra blank space characters' do - unsanitized = content.insert(100, "\n") - .insert(40, "\r\n") - .insert(30, ' ') - - sanitized = described_class.sanitize(unsanitized) - _, body = sanitized.split - - expect(sanitized).not_to eq(unsanitized) - expect(body).not_to match(/\s/) - end - end - - context "when key doesn't have blank space characters" do - it "doesn't modify the content" do - sanitized = described_class.sanitize(content) - - expect(sanitized).to eq(content) - end - end - - context "when key is invalid" do - it 'returns the original content' do - unsanitized = "ssh-foo any content==" - sanitized = described_class.sanitize(unsanitized) - - expect(sanitized).to eq(unsanitized) - end - end - end - describe '#valid?' do subject { public_key } diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index bf5703ac986..7398fd25aa8 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -72,52 +72,15 @@ describe Key, :mailer do expect(build(:key)).to be_valid end - it 'rejects the unfingerprintable key (not a key)' do - expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid - end - - where(:factory, :chars, :expected_sections) do - [ - [:key, ["\n", "\r\n"], 3], - [:key, [' ', ' '], 3], - [:key_without_comment, [' ', ' '], 2] - ] - end - - with_them do - let!(:key) { create(factory) } - let!(:original_fingerprint) { key.fingerprint } - - it 'accepts a key with blank space characters after stripping them' do - modified_key = key.key.insert(100, chars.first).insert(40, chars.last) - _, content = modified_key.split - - key.update!(key: modified_key) - - expect(key).to be_valid - expect(key.key.split.size).to eq(expected_sections) - - expect(content).not_to match(/\s/) - expect(original_fingerprint).to eq(key.fingerprint) - end - end - end - - context 'validate size' do - where(:key_content, :result) do - [ - [Spec::Support::Helpers::KeyGeneratorHelper.new(512).generate, false], - [Spec::Support::Helpers::KeyGeneratorHelper.new(8192).generate, false], - [Spec::Support::Helpers::KeyGeneratorHelper.new(1024).generate, true] - ] + it 'accepts a key with newline charecters after stripping them' do + key = build(:key) + key.key = key.key.insert(100, "\n") + key.key = key.key.insert(40, "\r\n") + expect(key).to be_valid end - with_them do - it 'validates the size of the key' do - key = build(:key, key: key_content) - - expect(key.valid?).to eq(result) - end + it 'rejects the unfingerprintable key (not a key)' do + expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid end end |