Address review comments

11 files changed, 29 insertions, 36 deletions

diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb
index 2e05b8c6d65..eeb130d5240 100644
--- a/app/helpers/form_helper.rb
+++ b/app/helpers/form_helper.rb
@@ -1,10 +1,10 @@
 module FormHelper
-  def form_errors(model, headline = 'The form contains the following')
+  def form_errors(model, type: 'form')
     return unless model.errors.any?
 
     pluralized = 'error'.pluralize(model.errors.count)
 
-    headline = headline + ' ' + pluralized + ':'
+    headline   = "The #{type} contains the following #{pluralized}:"
 
     content_tag(:div, class: 'alert alert-danger', id: 'error_explanation') do
       content_tag(:h4, headline) <<
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 0f9053262c2..fcf31694ab5 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -442,8 +442,7 @@ class ApplicationSetting < ActiveRecord::Base
   def key_restriction_for(type)
     attr_name = "#{type}_key_restriction"
 
-    # rubocop:disable GitlabSecurity/PublicSend
-    has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE
+    has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE # rubocop:disable GitlabSecurity/PublicSend
   end
 
   private
diff --git a/app/models/key.rb b/app/models/key.rb
index 2334603b58b..a6b4dcfec0d 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -1,7 +1,6 @@
 require 'digest/md5'
 
 class Key < ActiveRecord::Base
-  include AfterCommitQueue
   include Gitlab::CurrentSettings
   include Sortable
 
diff --git a/app/views/profiles/keys/_key_details.html.haml b/app/views/profiles/keys/_key_details.html.haml
index cf70320e7a8..77521417f47 100644
--- a/app/views/profiles/keys/_key_details.html.haml
+++ b/app/views/profiles/keys/_key_details.html.haml
@@ -16,7 +16,7 @@
           %strong= @key.last_used_at.try(:to_s, :medium) || 'N/A'
 
   .col-md-8
-    = form_errors(@key, 'The key has the following') unless @key.valid?
+    = form_errors(@key, type: 'key') unless @key.valid?
     %p
       %span.light Fingerprint:
       %code.key-fingerprint= @key.fingerprint
diff --git a/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml b/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml
index 2e0f509b17c..8ec78bbd41f 100644
--- a/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml
+++ b/changelogs/unreleased/17849-allow-admin-to-restrict-min-key-length-and-techno.yml
@@ -1,5 +1,5 @@
 ---
-title: Add settings for minimum key strength and allowed key type
+title: Add settings for minimum SSH key strength and allowed key type
 merge_request: 13712
 author: Cory Hinshaw
 type: added
diff --git a/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb b/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
index 2882e7f8b45..5b6079002c0 100644
--- a/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
+++ b/db/migrate/20161020180657_add_minimum_key_length_to_application_settings.rb
@@ -7,12 +7,13 @@ class AddMinimumKeyLengthToApplicationSettings < ActiveRecord::Migration
   disable_ddl_transaction!
 
   def up
-    # A key restriction has two possible states:
+    # A key restriction has these possible states:
     #
     #   * -1 means "this key type is completely disabled"
-    #   * >= 0 means "keys must have at least this many bits to be valid"
+    #   * 0 means "all keys of this type are valid"
+    #   * > 0 means "keys must have at least this many bits to be valid"
     #
-    # A value of 0 is equivalent to "there are no restrictions on keys of this type"
+    # The default is 0, for backward compatibility
     add_column_with_default :application_settings, :rsa_key_restriction, :integer, default: 0
     add_column_with_default :application_settings, :dsa_key_restriction, :integer, default: 0
     add_column_with_default :application_settings, :ecdsa_key_restriction, :integer, default: 0
diff --git a/doc/security/README.md b/doc/security/README.md
index 1f54948d113..0fea6be8b55 100644
--- a/doc/security/README.md
+++ b/doc/security/README.md
@@ -1,7 +1,7 @@
 # Security
 
 - [Password length limits](password_length_limits.md)
-- [Restrict allowed SSH key technologies and minimum length](ssh_keys_restrictions.md)
+- [Restrict SSH key technologies and minimum length](ssh_keys_restrictions.md)
 - [Rack attack](rack_attack.md)
 - [Webhooks and insecure internal web services](webhooks.md)
 - [Information exclusivity](information_exclusivity.md)
diff --git a/doc/security/ssh_keys_restrictions.md b/doc/security/ssh_keys_restrictions.md
index 32ca7dacab3..213fa5bfef5 100644
--- a/doc/security/ssh_keys_restrictions.md
+++ b/doc/security/ssh_keys_restrictions.md
@@ -2,12 +2,13 @@
 
 `ssh-keygen` allows users to create RSA keys with as few as 768 bits, which
 falls well below recommendations from certain standards groups (such as the US
-NIST). Some organizations deploying Gitlab will need to enforce minimum key
+NIST). Some organizations deploying GitLab will need to enforce minimum key
 strength, either to satisfy internal security policy or for regulatory
 compliance.
 
-Similarly, certain standards groups recommend using RSA or ECDSA over the older
-DSA and administrators may need to limit the allowed SSH key algorithms.
+Similarly, certain standards groups recommend using RSA, ECDSA, or ED25519 over
+the older DSA, and administrators may need to limit the allowed SSH key
+algorithms.
 
 GitLab allows you to restrict the allowed SSH key technology as well as specify
 the minimum key length for each technology.
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 9eab26d111e..62d1ecae676 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -34,8 +34,8 @@ module Gitlab
     end
 
     def check(cmd, changes)
-      check_valid_actor!
       check_protocol!
+      check_valid_actor!
       check_active_user!
       check_project_accessibility!
       check_project_moved!
diff --git a/lib/gitlab/ssh_public_key.rb b/lib/gitlab/ssh_public_key.rb
index a3f8730fb04..89ca1298120 100644
--- a/lib/gitlab/ssh_public_key.rb
+++ b/lib/gitlab/ssh_public_key.rb
@@ -13,6 +13,10 @@ module Gitlab
       Technologies.find { |tech| tech.name.to_s == name.to_s }
     end
 
+    def self.technology_for_key(key)
+      Technologies.find { |tech| key.is_a?(tech.key_class) }
+    end
+
     def self.supported_sizes(name)
       technology(name)&.supported_sizes
     end
@@ -37,9 +41,7 @@ module Gitlab
     end
 
     def type
-      return unless valid?
-
-      technology.name
+      technology.name if valid?
     end
 
     def bits
@@ -63,12 +65,7 @@ module Gitlab
 
     def technology
       @technology ||=
-        begin
-          tech = Technologies.find { |tech| key.is_a?(tech.key_class) }
-          raise "Unsupported key type: #{key.class}" unless tech
-
-          tech
-        end
+        self.class.technology_for_key(key) || raise("Unsupported key type: #{key.class}")
     end
   end
 end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 9e4174ecdca..458627ee4de 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -165,12 +165,10 @@ describe Gitlab::GitAccess do
         stub_application_setting(rsa_key_restriction: 4096)
       end
 
-      it 'does not allow keys which are too small' do
-        aggregate_failures do
-          expect(actor).not_to be_valid
-          expect { pull_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
-          expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
-        end
+      it 'does not allow keys which are too small', aggregate_failures: true do
+        expect(actor).not_to be_valid
+        expect { pull_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
+        expect { push_access_check }.to raise_unauthorized('Your SSH key must be at least 4096 bits.')
       end
     end
 
@@ -179,12 +177,10 @@ describe Gitlab::GitAccess do
         stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE)
       end
 
-      it 'does not allow keys which are too small' do
-        aggregate_failures do
-          expect(actor).not_to be_valid
-          expect { pull_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
-          expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
-        end
+      it 'does not allow keys which are too small', aggregate_failures: true do
+        expect(actor).not_to be_valid
+        expect { pull_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
+        expect { push_access_check }.to raise_unauthorized(/Your SSH key type is forbidden/)
       end
     end
   end