Merge branch '36776-entropy-requirements-for-new-user-passwords-mvc' into 'master'

Resolve "Allow to set a minimum password length via admin UI"

See merge request gitlab-org/gitlab!20661

app/helpers/application_settings_helper.rb

@@ -232,6 +232,7 @@ module ApplicationSettingsHelper
       :metrics_port,
       :metrics_sample_interval,
       :metrics_timeout,
+      :minimum_password_length,
       :mirror_available,
       :pages_domain_verification_enabled,
       :password_authentication_enabled_for_web,

app/models/application_setting.rb

@@ -46,6 +46,12 @@ class ApplicationSetting < ApplicationRecord
             presence: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
 
+  validates :minimum_password_length,
+            presence: true,
+            numericality: { only_integer: true,
+                            greater_than_or_equal_to: DEFAULT_MINIMUM_PASSWORD_LENGTH,
+                            less_than_or_equal_to: Devise.password_length.max }
+
   validates :home_page_url,
             allow_blank: true,
             addressable_url: true,

app/models/application_setting_implementation.rb

@@ -30,6 +30,8 @@ module ApplicationSettingImplementation
     '/admin/session'
   ].freeze
 
+  DEFAULT_MINIMUM_PASSWORD_LENGTH = 8
+
   class_methods do
     def defaults
       {
@@ -106,6 +108,7 @@ module ApplicationSettingImplementation
         sourcegraph_enabled: false,
         sourcegraph_url: nil,
         sourcegraph_public_only: true,
+        minimum_password_length: DEFAULT_MINIMUM_PASSWORD_LENGTH,
         terminal_max_session_time: 0,
         throttle_authenticated_api_enabled: false,
         throttle_authenticated_api_period_in_seconds: 3600,

app/models/user.rb

@@ -381,6 +381,11 @@ class User < ApplicationRecord
   # Class methods
   #
   class << self
+    # Devise method overridden to allow support for dynamic password lengths
+    def password_length
+      Gitlab::CurrentSettings.minimum_password_length..Devise.password_length.max
+    end
+
     # Devise method overridden to allow sign in with email or username
     def find_for_database_authentication(warden_conditions)
       conditions = warden_conditions.dup

app/services/users/build_service.rb

@@ -23,7 +23,7 @@ module Users
         @reset_token = user.generate_reset_token if params[:reset_password]
 
         if user_params[:force_random_password]
-          random_password = Devise.friendly_token.first(Devise.password_length.min)
+          random_password = Devise.friendly_token.first(User.password_length.min)
           user.password = user.password_confirmation = random_password
         end
       end

app/views/admin/application_settings/_signup.html.haml

@@ -12,6 +12,12 @@
         = f.check_box :send_user_confirmation_email, class: 'form-check-input'
         = f.label :send_user_confirmation_email, class: 'form-check-label' do
           Send confirmation email on sign-up
+    .form-group
+      = f.label :minimum_password_length, _('Minimum password length (number of characters)'), class: 'label-bold'
+      = f.number_field :minimum_password_length, class: 'form-control', rows: 4, min: ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH, max: Devise.password_length.max
+      - password_policy_guidelines_link = link_to _('Password Policy Guidelines'), 'https://about.gitlab.com/handbook/security/#gitlab-password-policy-guidelines', target: '_blank', rel: 'noopener noreferrer nofollow'
+      .form-text.text-muted
+        = _("See GitLab's %{password_policy_guidelines}").html_safe % { password_policy_guidelines: password_policy_guidelines_link }
     .form-group
       = f.label :domain_whitelist, 'Whitelisted domains for sign-ups', class: 'label-bold'
       = f.text_area :domain_whitelist_raw, placeholder: 'domain.com', class: 'form-control', rows: 8

changelogs/unreleased/36776-entropy-requirements-for-new-user-passwords-mvc.yml

+---
+title: Allow administrators to set a minimum password length
+merge_request: 20661
+author:
+type: added

config/initializers/devise_dynamic_password_length_validation.rb

+# frozen_string_literal: true
+
+# Discard the default Devise length validation from the `User` model.
+
+# This needs to be discarded because the length validation provided by Devise does not
+# support dynamically checking for min and max lengths.
+
+# A new length validation has been added to the User model instead, to keep supporting
+# dynamic password length validations, like:
+
+# validates :password, length: { maximum: proc { password_length.max }, minimum: proc { password_length.min } }, allow_blank: true
+
+def length_validator_supports_dynamic_length_checks?(validator)
+  validator.options[:minimum].is_a?(Proc) &&
+  validator.options[:maximum].is_a?(Proc)
+end
+
+# Get the in-built Devise validator on password length.
+password_length_validator = User.validators_on(:password).find do |validator|
+  validator.kind == :length
+end
+
+# This initializer can be removed as soon as https://github.com/plataformatec/devise/pull/5166
+# is merged into Devise.
+if length_validator_supports_dynamic_length_checks?(password_length_validator)
+  raise "Devise now supports dynamic length checks, please remove the monkey patch in #{__FILE__}"
+else
+  # discard the in-built length validator by always returning true
+  def password_length_validator.validate(*_)
+    true
+  end
+
+  # add a custom password length validator with support for dynamic length validation.
+  User.class_eval do
+    validates :password, length: { maximum: proc { password_length.max }, minimum: proc { password_length.min } }, allow_blank: true
+  end
+end

db/migrate/20191123062354_add_minimum_password_length_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddMinimumPasswordLengthToApplicationSettings < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  DEFAULT_MINIMUM_PASSWORD_LENGTH = 8
+
+  def change
+    add_column(:application_settings, :minimum_password_length, :integer, default: DEFAULT_MINIMUM_PASSWORD_LENGTH, null: false)
+  end
+end

db/post_migrate/20191205084057_update_minimum_password_length.rb

+# frozen_string_literal: true
+
+class UpdateMinimumPasswordLength < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    value_to_be_updated_to = [
+      Devise.password_length.min,
+      ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH
+    ].max
+
+    execute "UPDATE application_settings SET minimum_password_length = #{value_to_be_updated_to}"
+
+    ApplicationSetting.expire
+  end
+
+  def down
+    value_to_be_updated_to = ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH
+
+    execute "UPDATE application_settings SET minimum_password_length = #{value_to_be_updated_to}"
+
+    ApplicationSetting.expire
+  end
+end

db/schema.rb

@@ -351,6 +351,7 @@ ActiveRecord::Schema.define(version: 2019_12_08_071112) do
     t.string "sourcegraph_url", limit: 255
     t.boolean "sourcegraph_public_only", default: true, null: false
     t.bigint "snippet_size_limit", default: 52428800, null: false
+    t.integer "minimum_password_length", default: 8, null: false
     t.text "encrypted_akismet_api_key"
     t.string "encrypted_akismet_api_key_iv", limit: 255
     t.text "encrypted_elasticsearch_aws_secret_access_key"

doc/security/password_length_limits.md

@@ -4,7 +4,19 @@ type: reference, howto
 
 # Custom password length limits
 
-The user password length is set to a minimum of 8 characters by default.
+By default, GitLab supports passwords with:
+
+- A minimum length of 8.
+- A maximum length of 128.
+
+GitLab administrators can modify password lengths:
+
+- Using configuration file.
+- [From](https://gitlab.com/gitlab-org/gitlab/merge_requests/20661) GitLab 12.6, using the GitLab UI.
+
+## Modify maximum password length using configuration file
+
+The user password length is set to a maximum of 128 characters by default.
 To change that for installations from source:
 
 1. Edit `devise_password_length.rb`:
@@ -18,15 +30,35 @@ To change that for installations from source:
 1. Change the new password length limits:
 
    ```ruby
-   config.password_length = 12..128
+   config.password_length = 12..135
    ```
 
    In this example, the minimum length is 12 characters, and the maximum length
-   is 128 characters.
+   is 135 characters.
 
 1. [Restart GitLab](../administration/restart_gitlab.md#installations-from-source)
    for the changes to take effect.
 
+NOTE: **Note:**
+From GitLab 12.6, the minimum password length set in this configuration file will be ignored. Minimum password lengths will now have to be modified via the [GitLab UI](#modify-minimum-password-length-using-gitlab-ui) instead.
+
+## Modify minimum password length using GitLab UI
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/merge_requests/20661) in GitLab 12.6
+
+The user password length is set to a minimum of 8 characters by default.
+To change that using GitLab UI:
+
+In the Admin area under **Settings** (`/admin/application_settings`), go to section **Sign-up Restrictions**.
+
+[Minimum password length settings](../user/admin_area/img/minimum_password_length_settings_v12_6.png)
+
+Set the **Minimum password length** to a value greater than or equal to 8 and hit **Save changes** to save the changes.
+
+CAUTION: **Caution:**
+Changing minimum or maximum limit does not affect existing user passwords in any manner. Existing users will not be asked to reset their password to adhere to the new limits.
+The new limit restriction will only apply during new user sign-ups and when an existing user performs a password reset.
+
 <!-- ## Troubleshooting
 
 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

doc/user/admin_area/settings/sign_up_restrictions.md

@@ -19,6 +19,13 @@ their email address before they are allowed to sign in.
 
 ![Email confirmation](img/email_confirmation.png)
 
+## Minimum password length limit
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/merge_requests/20661) in GitLab 12.6
+
+You can [change](../../../security/password_length_limits.md#modify-minimum-password-length-using-gitlab-ui)
+the minimum number of characters a user must have in their password using the GitLab UI.
+
 ## Whitelist email domains
 
 > [Introduced][ce-598] in GitLab 7.11.0

ee/lib/ee/gitlab/scim/provisioning_service.rb

@@ -77,7 +77,7 @@ module EE
         end
 
         def random_password
-          Devise.friendly_token.first(Devise.password_length.min)
+          Devise.friendly_token.first(::User.password_length.min)
         end
 
         def valid_username

locale/gitlab.pot

@@ -11353,6 +11353,9 @@ msgstr ""
 msgid "Minimum length is %{minimum_password_length} characters."
 msgstr ""
 
+msgid "Minimum password length (number of characters)"
+msgstr ""
+
 msgid "Minutes"
 msgstr ""
 
@@ -12509,6 +12512,9 @@ msgstr ""
 msgid "Password (optional)"
 msgstr ""
 
+msgid "Password Policy Guidelines"
+msgstr ""
+
 msgid "Password authentication is unavailable."
 msgstr ""
 
@@ -15799,6 +15805,9 @@ msgstr ""
 msgid "SecurityDashboard|Unable to add %{invalidProjects}"
 msgstr ""
 
+msgid "See GitLab's %{password_policy_guidelines}"
+msgstr ""
+
 msgid "See metrics"
 msgstr ""
 

spec/controllers/admin/application_settings_controller_spec.rb

@@ -95,6 +95,13 @@ describe Admin::ApplicationSettingsController do
       expect(ApplicationSetting.current.default_project_creation).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS)
     end
 
+    it 'updates minimum_password_length setting' do
+      put :update, params: { application_setting: { minimum_password_length: 10 } }
+
+      expect(response).to redirect_to(admin_application_settings_path)
+      expect(ApplicationSetting.current.minimum_password_length).to eq(10)
+    end
+
     context 'external policy classification settings' do
       let(:settings) do
         {

spec/migrations/update_minimum_password_length_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191205084057_update_minimum_password_length')
+
+describe UpdateMinimumPasswordLength, :migration do
+  let(:application_settings) { table(:application_settings) }
+  let(:application_setting) do
+    application_settings.create!(
+      minimum_password_length: ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH
+    )
+  end
+
+  before do
+    stub_const('ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH', 10)
+    allow(Devise.password_length).to receive(:min).and_return(12)
+  end
+
+  it 'correctly migrates minimum_password_length' do
+    reversible_migration do |migration|
+      migration.before -> {
+        expect(application_setting.reload.minimum_password_length).to eq(10)
+      }
+
+      migration.after -> {
+        expect(application_setting.reload.minimum_password_length).to eq(12)
+      }
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -68,6 +68,12 @@ describe ApplicationSetting do
 
     it { is_expected.to validate_numericality_of(:snippet_size_limit).only_integer.is_greater_than(0) }
 
+    it { is_expected.not_to allow_value(7).for(:minimum_password_length) }
+    it { is_expected.not_to allow_value(129).for(:minimum_password_length) }
+    it { is_expected.not_to allow_value(nil).for(:minimum_password_length) }
+    it { is_expected.not_to allow_value('abc').for(:minimum_password_length) }
+    it { is_expected.to allow_value(10).for(:minimum_password_length) }
+
     context 'when snowplow is enabled' do
       before do
         setting.snowplow_enabled = true

spec/models/user_spec.rb

@@ -98,6 +98,53 @@ describe User, :do_not_mock_admin_mode do
   end
 
   describe 'validations' do
+    describe 'password' do
+      let!(:user) { create(:user) }
+
+      before do
+        allow(Devise).to receive(:password_length).and_return(8..128)
+        allow(described_class).to receive(:password_length).and_return(10..130)
+      end
+
+      context 'length' do
+        it { is_expected.to validate_length_of(:password).is_at_least(10).is_at_most(130) }
+      end
+
+      context 'length validator' do
+        context 'for a short password' do
+          before do
+            user.password = user.password_confirmation = 'abc'
+          end
+
+          it 'does not run the default Devise password length validation' do
+            expect(user).to be_invalid
+            expect(user.errors.full_messages.join).not_to include('is too short (minimum is 8 characters)')
+          end
+
+          it 'runs the custom password length validator' do
+            expect(user).to be_invalid
+            expect(user.errors.full_messages.join).to include('is too short (minimum is 10 characters)')
+          end
+        end
+
+        context 'for a long password' do
+          before do
+            user.password = user.password_confirmation = 'a' * 140
+          end
+
+          it 'does not run the default Devise password length validation' do
+            expect(user).to be_invalid
+            expect(user.errors.full_messages.join).not_to include('is too long (maximum is 128 characters)')
+          end
+
+          it 'runs the custom password length validator' do
+            expect(user).to be_invalid
+            expect(user.errors.full_messages.join).to include('is too long (maximum is 130 characters)')
+          end
+        end
+      end
+    end
+
     describe 'name' do
       it { is_expected.to validate_presence_of(:name) }
       it { is_expected.to validate_length_of(:name).is_at_most(128) }
@@ -461,6 +508,34 @@ describe User, :do_not_mock_admin_mode do
       end
     end
 
+    describe '.password_length' do
+      let(:password_length) { described_class.password_length }
+
+      it 'is expected to be a Range' do
+        expect(password_length).to be_a(Range)
+      end
+
+      context 'minimum value' do
+        before do
+          stub_application_setting(minimum_password_length: 101)
+        end
+
+        it 'is determined by the current value of `minimum_password_length` attribute of application_setting' do
+          expect(password_length.min).to eq(101)
+        end
+      end
+
+      context 'maximum value' do
+        before do
+          allow(Devise.password_length).to receive(:max).and_return(201)
+        end
+
+        it 'is determined by the current value of `Devise.password_length.max`' do
+          expect(password_length.max).to eq(201)
+        end
+      end
+    end
+
     describe '.limit_to_todo_authors' do
       context 'when filtering by todo authors' do
         let(:user1) { create(:user) }