From 21a582787d080ac515245b6c547342e99b15a193 Mon Sep 17 00:00:00 2001 From: manojmj Date: Sat, 23 Nov 2019 16:53:10 +0530 Subject: [PATCH 1/8] Add new minimum password requirements This change adds the ability for the admin to configure new minimum password requirements --- app/helpers/application_settings_helper.rb | 1 + app/models/application_setting.rb | 8 + app/models/concerns/devise_validatable.rb | 35 +++++ app/models/user.rb | 9 +- app/services/users/build_service.rb | 2 +- .../application_settings/_signup.html.haml | 6 + ...equirements-for-new-user-passwords-mvc.yml | 5 + config/initializers/8_devise.rb | 4 +- config/initializers/devise_controller.rb | 10 ++ ...password_length_to_application_settings.rb | 15 ++ db/schema.rb | 1 + ee/lib/ee/gitlab/scim/provisioning_service.rb | 2 +- locale/gitlab.pot | 6 + .../application_settings_controller_spec.rb | 7 + spec/models/application_setting_spec.rb | 6 + .../concerns/devise_validatable_spec.rb | 141 ++++++++++++++++++ spec/models/user_spec.rb | 28 ++++ 17 files changed, 281 insertions(+), 5 deletions(-) create mode 100644 app/models/concerns/devise_validatable.rb create mode 100644 changelogs/unreleased/36776-entropy-requirements-for-new-user-passwords-mvc.yml create mode 100644 config/initializers/devise_controller.rb create mode 100644 db/migrate/20191123062354_add_minimum_password_length_to_application_settings.rb create mode 100644 spec/models/concerns/devise_validatable_spec.rb diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index d9416cb10c42..71e4195c50fc 100644 --- a/app/helpers/application_settings_helper.rb +++ b/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, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index e764b6c56b05..238fc0881c11 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -6,6 +6,8 @@ class ApplicationSetting < ApplicationRecord include TokenAuthenticatable include ChronicDurationAttribute + DEFAULT_MINIMUM_PASSWORD_LENGTH = 8 + add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required } add_authentication_token_field :health_check_access_token add_authentication_token_field :static_objects_external_storage_auth_token @@ -46,6 +48,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: DEFAULT_MAXIMUM_PASSWORD_LENGTH } + validates :home_page_url, allow_blank: true, addressable_url: true, diff --git a/app/models/concerns/devise_validatable.rb b/app/models/concerns/devise_validatable.rb new file mode 100644 index 000000000000..6fc7cf56439a --- /dev/null +++ b/app/models/concerns/devise_validatable.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +# Validations in this module are copied over from +# https://github.com/plataformatec/devise/blob/v4.7.1/lib/devise/models/validatable.rb + +# TODO: Remove this concern and go back to using Devise's in built Validatable module as soon as +# https://github.com/plataformatec/devise/pull/5166 is merged. +module DeviseValidatable + extend ActiveSupport::Concern + + # rubocop: disable Rails/Validation + included do + validates_presence_of :email, if: :email_required? + validates_uniqueness_of :email, allow_blank: true, case_sensitive: true, if: :will_save_change_to_email? + validates_format_of :email, with: Devise.email_regexp, allow_blank: true, if: :will_save_change_to_email? + + validates_presence_of :password, if: :password_required? + validates_confirmation_of :password, if: :password_required? + + # This is the only validation that differs from the validations already present in Devise's Validatable module. + # We need this validation so that we can support dynamic password length without a GitLab restart. + validates_length_of :password, maximum: proc { password_length.max }, minimum: proc { password_length.min }, allow_blank: true + end + # rubocop: enable Rails/Validation + + private + + def password_required? + !persisted? || !password.nil? || !password_confirmation.nil? + end + + def email_required? + true + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 698848c5b16a..6589fb96aa19 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,6 +20,7 @@ class User < ApplicationRecord include WithUploads include OptionallySearch include FromUnion + include DeviseValidatable DEFAULT_NOTIFICATION_LEVEL = :participating @@ -51,7 +52,7 @@ class User < ApplicationRecord serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize devise :lockable, :recoverable, :rememberable, :trackable, - :validatable, :omniauthable, :confirmable, :registerable + :omniauthable, :confirmable, :registerable BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \ "administrator if you think this is an error." @@ -158,7 +159,7 @@ class User < ApplicationRecord # # Validations # - # Note: devise :validatable above adds validations for :email and :password + # Note: DeviseValidatable adds validations for :email and :password validates :name, presence: true, length: { maximum: 128 } validates :first_name, length: { maximum: 255 } validates :last_name, length: { maximum: 255 } @@ -1751,6 +1752,10 @@ class User < ApplicationRecord def no_recent_activity? last_active_at.to_i <= MINIMUM_INACTIVE_DAYS.days.ago.to_i end + + def self.password_length + Gitlab::CurrentSettings.minimum_password_length..ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH + end end User.prepend_if_ee('EE::User') diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb index 8c85ad9ffd8b..ea4d11e728e0 100644 --- a/app/services/users/build_service.rb +++ b/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 diff --git a/app/views/admin/application_settings/_signup.html.haml b/app/views/admin/application_settings/_signup.html.haml index 7c1df78f30c9..6ec214fceb0e 100644 --- a/app/views/admin/application_settings/_signup.html.haml +++ b/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, 'Password Requirements', class: 'label-bold' + = f.text_field :minimum_password_length, placeholder: 'minimum password length', class: 'form-control', rows: 4 + - password_policy_guidelines_link = link_to _('Password Policy Guidelines'), 'https://about.gitlab.com/handbook/security/#gitlab-password-policy-guidelines', target: '_blank' + .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 diff --git a/changelogs/unreleased/36776-entropy-requirements-for-new-user-passwords-mvc.yml b/changelogs/unreleased/36776-entropy-requirements-for-new-user-passwords-mvc.yml new file mode 100644 index 000000000000..6b2e159c273e --- /dev/null +++ b/changelogs/unreleased/36776-entropy-requirements-for-new-user-passwords-mvc.yml @@ -0,0 +1,5 @@ +--- +title: Allow administrators to set a minimum password length +merge_request: 20661 +author: +type: added diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index 8d4c5fa382c5..0a1b3c90145d 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -113,7 +113,9 @@ Devise.setup do |config| # ==> Configuration for :validatable # Range for password length. Default is 6..128. - config.password_length = 8..128 + # config.password_length = 8..128 + # Devise's `validatable` module is no longer used. + # Dynamic minimum password length is supported by DeviseValidatable concern. # Email regex used to validate email formats. It simply asserts that # an one (and only one) @ exists in the given string. This is mainly diff --git a/config/initializers/devise_controller.rb b/config/initializers/devise_controller.rb new file mode 100644 index 000000000000..53baf45dda3a --- /dev/null +++ b/config/initializers/devise_controller.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +DeviseController.class_eval do + # Overriding set_minimum_password_length from DeviseController. + + # Sets minimum password length to show to user + def set_minimum_password_length + @minimum_password_length = resource_class.password_length.min + end +end diff --git a/db/migrate/20191123062354_add_minimum_password_length_to_application_settings.rb b/db/migrate/20191123062354_add_minimum_password_length_to_application_settings.rb new file mode 100644 index 000000000000..809cd5ad3be9 --- /dev/null +++ b/db/migrate/20191123062354_add_minimum_password_length_to_application_settings.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddMinimumPasswordLengthToApplicationSettings < ActiveRecord::Migration[5.2] + DOWNTIME = false + + DEFAULT_MINIMUM_PASSWORD_LENGTH = 8 + + def up + add_column(:application_settings, :minimum_password_length, :integer, default: DEFAULT_MINIMUM_PASSWORD_LENGTH, null: false) + end + + def down + remove_column(:application_settings, :minimum_password_length) + end +end diff --git a/db/schema.rb b/db/schema.rb index ac2372a63d09..93b0c54a2e62 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -363,6 +363,7 @@ ActiveRecord::Schema.define(version: 2019_12_08_071112) do t.string "encrypted_slack_app_secret_iv", limit: 255 t.text "encrypted_slack_app_verification_token" t.string "encrypted_slack_app_verification_token_iv", limit: 255 + t.integer "minimum_password_length", default: 8, null: false t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id" t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id" diff --git a/ee/lib/ee/gitlab/scim/provisioning_service.rb b/ee/lib/ee/gitlab/scim/provisioning_service.rb index 2eaa5ee9b9af..02bbc7fb2bc3 100644 --- a/ee/lib/ee/gitlab/scim/provisioning_service.rb +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 405d6948bade..7ef8b0ae2c68 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12464,6 +12464,9 @@ msgstr "" msgid "Password (optional)" msgstr "" +msgid "Password Policy Guidelines" +msgstr "" + msgid "Password authentication is unavailable." msgstr "" @@ -15742,6 +15745,9 @@ msgstr "" msgid "SecurityDashboard|Unable to add %{invalidProjects}" msgstr "" +msgid "See GitLab's %{password_policy_guidelines}" +msgstr "" + msgid "See metrics" msgstr "" diff --git a/spec/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb index bc14e9112a15..fa575ba2eae1 100644 --- a/spec/controllers/admin/application_settings_controller_spec.rb +++ b/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 { diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 9a76be5b6f19..a403aa296d45 100644 --- a/spec/models/application_setting_spec.rb +++ b/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 diff --git a/spec/models/concerns/devise_validatable_spec.rb b/spec/models/concerns/devise_validatable_spec.rb new file mode 100644 index 000000000000..8ab3e10b3582 --- /dev/null +++ b/spec/models/concerns/devise_validatable_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DeviseValidatable do + describe 'User' do + let_it_be(:existing_user) { create(:user) } + + context 'password' do + it 'requires password to be set for a new record' do + user = build(:user, password: '', password_confirmation: '') + + expect(user).to be_invalid + expect(user.errors[:password].join).to eq('can\'t be blank') + end + + it 'requires password_confirmation to be set for a new record' do + user = build(:user, password: 'new_password', password_confirmation: 'something_else') + + expect(user).to be_invalid + expect(user.errors[:password_confirmation].join).to eq('doesn\'t match Password') + end + + it 'requires password to be set while updating/resetting password' do + existing_user.password = '' + existing_user.password_confirmation = '' + + expect(existing_user).to be_invalid + expect(existing_user.errors[:password].join).to eq('can\'t be blank') + end + + it 'requires password_confirmation to be set while updating/resetting password' do + existing_user.password = 'set_new_password' + existing_user.password_confirmation = 'something_else' + + expect(existing_user).to be_invalid + expect(existing_user.errors[:password_confirmation].join).to eq('doesn\'t match Password') + end + + it 'does not fail validations for password length when password is not changed' do + existing_user.password = existing_user.password_confirmation = nil + + expect(existing_user).to be_valid + end + + context 'password length' do + before do + expect(User).to receive(:password_length).twice.and_return(10..128) + end + + it 'validates minimum password length' do + user = build(:user, password: 'x' * 9, password_confirmation: 'x' * 9) + + expect(user).to be_invalid + expect(user.errors[:password].join).to eq('is too short (minimum is 10 characters)') + end + + it 'validates maximum password length' do + user = build(:user, password: 'x' * 129, password_confirmation: 'x' * 129) + + expect(user).to be_invalid + expect(user.errors[:password].join).to eq('is too long (maximum is 128 characters)') + end + + it 'validates password length even if password is not required' do + user = build(:user, password: 'x' * 129, password_confirmation: 'x' * 129) + expect(user).to receive(:password_required?).twice.and_return(false) + + expect(user).to be_invalid + expect(user.errors[:password].join).to eq('is too long (maximum is 128 characters)') + end + + context 'min and max length values' do + it 'matches the values set by the `password_length` method' do + validators = User.validators_on :password + length = validators.find { |v| v.kind == :length } + + assert_equal 10, length.options[:minimum].call + assert_equal 128, length.options[:maximum].call + end + end + end + end + + context 'email' do + it 'requires email to be set' do + user = build(:user, email: nil) + + expect(user).to be_invalid + expect(user.errors[:email].join).to eq('can\'t be blank') + end + + it 'accepts valid formats' do + %w(a.b.c@example.com test_mail@gmail.com any@any.net email@test.br 123@mail.test 1☃3@mail.test).each do |email| + user = build(:user, email: email) + + expect(user).to be_valid + expect(user.errors[:email]).to be_blank + end + end + + it 'is case-senstive' do + user = build(:user, email: 'test@test.com') + + expect(user).to be_valid + + new_user = build(:user, email: 'TEST@TEST.COM') + + expect(new_user).to be_valid + expect(new_user.errors[:email].join).to be_blank + end + + context 'when email changes' do + it 'validates uniqueness, allowing blank' do + user = build(:user, email: '') + + expect(user).to be_invalid + expect(user.errors[:email].join).not_to include('taken') + + user.email = existing_user.email + + expect(user).to be_invalid + expect(user.errors[:email].join).to include('taken') + end + + it 'validates format, allowing blank' do + user = build(:user, email: '') + + expect(user).to be_invalid + expect(user.errors[:email].join).not_to include('invalid') + + %w{invalid_email_format 123 $$$ () ☃}.each do |email| + user.email = email + expect(user).to be_invalid + expect(user.errors[:email].join).to include('invalid') + end + end + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 49991dbd2d49..fafb21a29908 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -461,6 +461,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 + stub_const('ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH', 201) + end + + it 'is determined by the current value of `ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH`' 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) } -- GitLab From 89376a90b950ca5ba8e162320b6a58601e4ae5f1 Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 2 Dec 2019 10:44:01 +0530 Subject: [PATCH 2/8] Do not use DeviseValidatable module This change removes the DeviseValidatable module --- app/models/application_setting.rb | 4 +- .../application_setting_implementation.rb | 3 + app/models/concerns/devise_validatable.rb | 35 ----- app/models/user.rb | 8 +- .../application_settings/_signup.html.haml | 6 +- config/initializers/8_devise.rb | 4 +- config/initializers/devise_controller.rb | 10 -- ...evise_remove_password_length_validation.rb | 20 +++ db/schema.rb | 2 +- doc/security/password_length_limits.md | 37 ++++- .../img/minimum_password_length.png | Bin 0 -> 27994 bytes .../settings/sign_up_restrictions.md | 6 + locale/gitlab.pot | 3 + .../concerns/devise_validatable_spec.rb | 141 ------------------ spec/models/user_spec.rb | 31 +++- 15 files changed, 105 insertions(+), 205 deletions(-) delete mode 100644 app/models/concerns/devise_validatable.rb delete mode 100644 config/initializers/devise_controller.rb create mode 100644 config/initializers/devise_remove_password_length_validation.rb create mode 100644 doc/user/admin_area/img/minimum_password_length.png delete mode 100644 spec/models/concerns/devise_validatable_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 238fc0881c11..456b64300885 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -6,8 +6,6 @@ class ApplicationSetting < ApplicationRecord include TokenAuthenticatable include ChronicDurationAttribute - DEFAULT_MINIMUM_PASSWORD_LENGTH = 8 - add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required } add_authentication_token_field :health_check_access_token add_authentication_token_field :static_objects_external_storage_auth_token @@ -52,7 +50,7 @@ class ApplicationSetting < ApplicationRecord presence: true, numericality: { only_integer: true, greater_than_or_equal_to: DEFAULT_MINIMUM_PASSWORD_LENGTH, - less_than_or_equal_to: DEFAULT_MAXIMUM_PASSWORD_LENGTH } + less_than_or_equal_to: Devise.password_length.max } validates :home_page_url, allow_blank: true, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index e1eb8d429bb9..98d8bb43b93f 100644 --- a/app/models/application_setting_implementation.rb +++ b/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, diff --git a/app/models/concerns/devise_validatable.rb b/app/models/concerns/devise_validatable.rb deleted file mode 100644 index 6fc7cf56439a..000000000000 --- a/app/models/concerns/devise_validatable.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -# Validations in this module are copied over from -# https://github.com/plataformatec/devise/blob/v4.7.1/lib/devise/models/validatable.rb - -# TODO: Remove this concern and go back to using Devise's in built Validatable module as soon as -# https://github.com/plataformatec/devise/pull/5166 is merged. -module DeviseValidatable - extend ActiveSupport::Concern - - # rubocop: disable Rails/Validation - included do - validates_presence_of :email, if: :email_required? - validates_uniqueness_of :email, allow_blank: true, case_sensitive: true, if: :will_save_change_to_email? - validates_format_of :email, with: Devise.email_regexp, allow_blank: true, if: :will_save_change_to_email? - - validates_presence_of :password, if: :password_required? - validates_confirmation_of :password, if: :password_required? - - # This is the only validation that differs from the validations already present in Devise's Validatable module. - # We need this validation so that we can support dynamic password length without a GitLab restart. - validates_length_of :password, maximum: proc { password_length.max }, minimum: proc { password_length.min }, allow_blank: true - end - # rubocop: enable Rails/Validation - - private - - def password_required? - !persisted? || !password.nil? || !password_confirmation.nil? - end - - def email_required? - true - end -end diff --git a/app/models/user.rb b/app/models/user.rb index 6589fb96aa19..50297bffe95c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,7 +20,6 @@ class User < ApplicationRecord include WithUploads include OptionallySearch include FromUnion - include DeviseValidatable DEFAULT_NOTIFICATION_LEVEL = :participating @@ -52,7 +51,7 @@ class User < ApplicationRecord serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize devise :lockable, :recoverable, :rememberable, :trackable, - :omniauthable, :confirmable, :registerable + :validatable, :omniauthable, :confirmable, :registerable BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \ "administrator if you think this is an error." @@ -159,7 +158,8 @@ class User < ApplicationRecord # # Validations # - # Note: DeviseValidatable adds validations for :email and :password + # Note: devise :validatable above adds validations for :email + validates :password, length: { maximum: proc { password_length.max }, minimum: proc { password_length.min } }, allow_blank: true validates :name, presence: true, length: { maximum: 128 } validates :first_name, length: { maximum: 255 } validates :last_name, length: { maximum: 255 } @@ -1754,7 +1754,7 @@ class User < ApplicationRecord end def self.password_length - Gitlab::CurrentSettings.minimum_password_length..ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH + Gitlab::CurrentSettings.minimum_password_length..Devise.password_length.max end end diff --git a/app/views/admin/application_settings/_signup.html.haml b/app/views/admin/application_settings/_signup.html.haml index 6ec214fceb0e..60765b63504d 100644 --- a/app/views/admin/application_settings/_signup.html.haml +++ b/app/views/admin/application_settings/_signup.html.haml @@ -13,9 +13,9 @@ = f.label :send_user_confirmation_email, class: 'form-check-label' do Send confirmation email on sign-up .form-group - = f.label :minimum_password_length, 'Password Requirements', class: 'label-bold' - = f.text_field :minimum_password_length, placeholder: 'minimum password length', class: 'form-control', rows: 4 - - password_policy_guidelines_link = link_to _('Password Policy Guidelines'), 'https://about.gitlab.com/handbook/security/#gitlab-password-policy-guidelines', target: '_blank' + = f.label :minimum_password_length, _('Minimum password length'), 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 diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index 0a1b3c90145d..8d4c5fa382c5 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -113,9 +113,7 @@ Devise.setup do |config| # ==> Configuration for :validatable # Range for password length. Default is 6..128. - # config.password_length = 8..128 - # Devise's `validatable` module is no longer used. - # Dynamic minimum password length is supported by DeviseValidatable concern. + config.password_length = 8..128 # Email regex used to validate email formats. It simply asserts that # an one (and only one) @ exists in the given string. This is mainly diff --git a/config/initializers/devise_controller.rb b/config/initializers/devise_controller.rb deleted file mode 100644 index 53baf45dda3a..000000000000 --- a/config/initializers/devise_controller.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -DeviseController.class_eval do - # Overriding set_minimum_password_length from DeviseController. - - # Sets minimum password length to show to user - def set_minimum_password_length - @minimum_password_length = resource_class.password_length.min - end -end diff --git a/config/initializers/devise_remove_password_length_validation.rb b/config/initializers/devise_remove_password_length_validation.rb new file mode 100644 index 000000000000..13c7dd0664a7 --- /dev/null +++ b/config/initializers/devise_remove_password_length_validation.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# Remove the default Devise length validation from the `User` model. + +# This needs to be removed 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 `models/user.rb` instead, to keep supporting +# dynamic length validations, like: + +# validates :password, length: { maximum: proc { password_length.max }, minimum: proc { password_length.min } }, allow_blank: true + +# This can be removed as soon as https://github.com/plataformatec/devise/pull/5166 +# is merged into Devise. + +User._validators[:password].delete_if do |validator| + validator.kind == :length && + validator.options[:minimum].is_a?(Integer) && + validator.options[:maximum].is_a?(Integer) +end diff --git a/db/schema.rb b/db/schema.rb index 93b0c54a2e62..9bc3554e86a1 100644 --- a/db/schema.rb +++ b/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" @@ -363,7 +364,6 @@ ActiveRecord::Schema.define(version: 2019_12_08_071112) do t.string "encrypted_slack_app_secret_iv", limit: 255 t.text "encrypted_slack_app_verification_token" t.string "encrypted_slack_app_verification_token_iv", limit: 255 - t.integer "minimum_password_length", default: 8, null: false t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id" t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id" diff --git a/doc/security/password_length_limits.md b/doc/security/password_length_limits.md index 9909ef4a8e4c..cd07cd94d310 100644 --- a/doc/security/password_length_limits.md +++ b/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,34 @@ 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.png) + +Set the **Minimum password length** to a value greater than or equal to 8 and hit **Save changes** to save the changes. + +>**Note**: 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. + m>kXhFJ6@F-yAY(a!-ukm%s1ql63)B&Vm z1xUaGFaW5xOcsDEoXNd(3i5iC`)5cyaPK>KeMV~TfJbW-s7am<0ES_ur_mr1;e8Vi1a7C&K&c22`2`zDmi!?x>|Yw(c1^` zN!;MH!@iAG5-1_PGb&ZbBr@tr7I&61Zpp)5b+-T0B333R|WqYFg}EAhE0JGV^T` zJq}U~qhXE}=xO)d1g3Ojv?xD=WXS@5V$QKoFML}i-V|4D8I&u`w=A@F6(EOsYYrMV z-U!HtF=9jN_3Iqg=z7P5aw~;o?LsNI$JkQ^;Tew_#S|)$Ps7R`i@1J2s3Cs-Duzk2 z$!X{y1WQO0sH?C7RYgUhXo!vJ8ZN0m2j=M@j-8;?slu#^7p?-|FhMd|W0dnw43gx2 zPkN9GtJhR5a&Lkv2-|6tJO0WBrQNx$fF#xiOi7ADI7DL38mgfv5|kOWA$?`@ax18C z82hT3w+2^zb4>s_-OQ{K`@JrJM6|6ql&XPKW&<__DMB*A)vs}=I%7xhGwtvN^6HC8 zq=>pWn)O+yBlt-{$%$r9{A4gk{=;->4REgJuqg-0NhC+s z5KuOm1ldbDGf8yBanpLl5Y)3~1EZ=w93JD9;CWd_nmXyO)pJaZ2TF~8u8nB`qi7Cs zFbGM@Lk+xAJ(h0FyJV;}G0yeUK3e1!RW*fgo^(G4Aj$rke1lAX<6!k1_K!vQRkZ_KQlB}}q@@0rzG#>#`gxewSNwc*^GOkC+)g9(H07qzxOFwwTPU%i6mo&V(j{!WSq4F@}igIml z8e>L;^I7{lNuNmZy?k2RvU=|XN#y*G_0F$3dafhQt1YpI{c4isE-`Ig$s)JcjPm1t zM>@#59x1~lxC54S5z|Bhaf|LvGPBuYCi{(ssYT}UW>6bN+e6v8SO9!Cy=;W{boqp$ zd@v7yAWQg8>D0I(%3PK)0?kxOH0~0Tv5pRF4pots*}TQksS0upQlQj9p#f~eL9Rj74*Lh4J>1gCYTA)q%$?Eu`}Rs zMa8%BKuHl(5Z$iHhWGMi%-Xkm#n$2T;xb(jfhD}lHjJP&LjDJ76JvU8)z@^-{WaS) zI_#kAZ~}M)+k56!jA)xq+1VQ^b0_8z*|yVYf>^LtLqeKj3>n-p(3E*fi^DlKs;4V# zmIDFh59E}NarbT?1qAz&zzfH#3!$?-T{bMUv3z3)iq7Pf6r{btlZGAeHqrd{AzCS; z@sK5^{AWU~uUE=~KuP3+#2?SAxCeX4Nq{r9CBNp+H%i{x_GS8&GFz5$q282>sVdEv zqr{=+EMhMz^P;N_ki_`7!Q@-|U+SpPgjeQ%LiLphewEPViM%AtgDC!_k~Bf~@G9?V z8AMtqBRjIG4l4S7fn992Y>SAvC(Qx&kT9j`m?A~9T^MS&NhOaOsJNiQk~?Fg=u{Ka zE^SDq(PH!LB`d$V+n#W3%2UB!^;eO0w2odS`G>#&StMR!9m#4)XE$-<@lT3dT87}z zE6k8{v*1onqWWe{n>USwvLu(rx^+gsjV*uK)%eDulPtyc_d?^9Ish|Vr$V!yT-d)s zRr4l+wUcoyJ!wdf;=h0fF8~@Ro;G)WbCdrEAGJvVXoN#{{a<#}GmH%En-?zsZ#ybb zj?JVpurK73;A(BP7yz_h-1nzpN*0C#n!lo`{>M|%VAzN8KvF)d7Nq%?9&QQ)&0h*3 z`h83O8iIQiPK&f zfH6CVX*V<`-`RU03uMDZPT$f9cOjP85=1q7z%IoTp8(Xc=A93A4jZR-i>`Dsd;?^# zXz23;?Hl>^zO)b+ZhRtY3)1O)5ZA*}fphCz=KsY6Q0FyP_5h`qjLQ5$1V{@E{V9nK zuA`qG0m2(Xai(M>OFNp!sN`mjbCYlFl|NQBpdb3B#l!Nz>PrL2npOyx1}#wNV+D#F z6W(g4n4QZhhMj)lQr!vd86vH<0HR~8FHYD3tQ-Lt@Dxbqmbdj&cb=jThm9uUs9)ba zwDA0V%;=?NcnmAQ8`$J70;v#T2TbnomvS5NNfvn{ymTJ0QD;n}V=zos zM*kKN-zClSl!C3lPN=Rj?k)9lC-mGS)OB!?|3rScU~|?ig7dyGpt#hBrGYG0Zp-U@$zq}#NTh2>#-OIuVL@K zJL88My#b&(CY+Jw{u?CpMz4%P&?WI6Pr5oi9WmW(`o7x|sVv*6w-p_1)Kxy>P`;dyW#WFBCDp#k*!@%C zPsgW0X|1OAO-66}Nez!F-Q%h)B=3NC3&XwXq`n2722Hd>_dCq5-^@`Lx^o4xXZro; zK=kSXi@8s1-}zUd`q5RKeEZ6C+5$%WItcbwJ!j45Y(ZjJ0`-p|CB*}wOHZ%M+BXyS zuqd^GxN}2wYw(EJ^2Vl2#f=dlZi3-ly1KW4`t&h=&J z{aH`K^y&DUj653?ztS@wW&CSMRQo9mM8hk<9RLY(AlXnpXg(;MIPE)k-5;V#facDp z`2wu~Wzd8%0{s#y&^{OfRV<(fB9l%!#WHc1jlDqfVS%fjW0s&%ZSC87+$hV@vTkg( z+LQ1>H|cAZal<-h=u3G|ePq&fgIb%il3hm1a-00dT)c*O+h;}M+G#cwZ+jP)PGUy3 zS-}?}J(TNd;=v%2Q{5VRPc&*RQ(XL4Y3m@DBHODk) z_T+)&f`g|pid)eP#L^M`*xXR9MLFcf^Y`G1`|+5RnOo@$exaOJABj}$uiXCWU%op0 zaQcfj(g!Wu0s42tb#U7CtL*jB967Qrx4u@hvQn zR;-&;-U}D0P&`FYrW$&qH`hBo{XOcC%kx+-^vbib>DS9XSq&eS@_Sb{)2n()-W@#)~4gqC$hdd9f&sdWVl+R@nC#aSTeDZ6iIT319h%IEwbda{eZvf(q+b*gdU zg*WQVEmR|dPO!KKU#w+a#Tn-?+Y|mn$6bul?^qylwNnND0Gr&=I5}m|Ejh8B9VcbcyuRdoxQMpeHjs=9SZc&r zCt8)Bm|N!VwJ|Q=`W|2T$#27~BhzkNX{|7jJ)+cw%7y8_%8IV)%j`X^lr3D$(<}=k)yXs-5Bh^`6 zeyek4n6mpd8uuhrvz?2xNe$JLk(vgby#gq&dW9w5V0Qrt#GIOy$ebYhJ?IK|0ek=7 z9iSKSkX{hl^Ujg9VIgr3&p-@7yexempssRa1%3N~Uj(tg#PJQllbQl#fH~q$3=8HH z-+g%x6Clqk#z)*1Fkb+~n?lfR>;Wc{DfQF$4{{E7)2tv4kAy|pcg9x!a}VU*k=98Z zw!J`7)OY3}=+xWc&a109(ZE9Y9Kz?QAF?_^hq!l``6+Xvbc5gPGgGCRtXu_@-2BXa z0prUw>4BASMma^ee>A=#*XQ7*`Fssl&acB@<#{SuMSVtPZ3c-d#k_KH7>%a6cHA+P zQ-Nc;a)TuboPwo_(+Zo){fzo%xX_t*%1-+Xd{ZV`tr+mG);D|mB;=U6YY6{D&jy}q zkd*htQuB>{I=4BaBZ_^F$T3*HXDbqi>*2m$i=@$(?b`-U?Wx3W$pS!VO`q*l@%7}q zA{F^48o9)&X}+^!@vid57jI!TH|9`LuZA3!MnVUZm?DDcRvS!Hv|8UJJtrEt78D7= z4MHpNhj7nL1P(4QWp*p>Zm6tc&n*fz&be&NbhbR8C4S9|7N1pkuc@PS%RE<;D9=Fo zYq(HZc#bY=^(V6YB>efjc(+{V_}%bea+0k*`JDDCM$I6LBXeY=oX0vpR^(0s6(z*6 zoe~n0jet#TAD}0&%m>b9k})GXYuCGE+CrxL3zs^v^um55J)ArAf^gmDEtxx zDFU7jxs=YPRJocgux^6B9XquKwi;@tL4VB*Y#G*|jo?pkGBu)zSPtt%Q&WJ6a4qh` z9B7CTo`KrPLoZ=qGa}Ch1B7>_OOh64B_1QY>zeBG0;KDkHx-n8nyW_cR%=`nl2N*D zwN=)(9xT(xMbGo-nc)R;&^;^w+j)CJ|;#phRkwr>HOnE`l8B|Job3|yp-nAj(`f~tXPlPZvVo0cTKA0S>u*(knU{J0 z{?t2`bn@mKm+7~GE@URDZMd%xyF!@Y@im9fT#9A7*$2`5%>=XE?CvmW;*0*lFzHJl zEq?}tW#T!df|*$sozUq%S>uyN+4^54Stw&$hXo^a>cy6;4y?!$iCs^tX=jEv{95ng zux<&1D!0}aFf;4;fa?}L)YzeA&q2e$=cN9S6`#_zweXJ*{wfMQVpxZs7>j_IuuS@F-FJYTaui(R4Va6NZ6$zoxKx%?!PCxHOHhOF;TVVvk%Gs3r(pLY zxOj4W!`epV-9u&glciLaF~mRCQgNlwg_8Y7jd0VS_?S z*y;zn2M#yY&+Flp$SqBP7?qOLIZmyUOg%`CK)NektHRvy9}myugzZz(wnHT&e1|9u zFX>STKzbBWwMl)}Dj<6jc387|=meh};ahuYyIysqc^ocZeoYC{ zc7IIEO6l==2z$l+;&!MLe_4LMYLO6f0-k)gSg5eEXJp(JvHIl0^tbAQd$6B^CUPw5 zr?4!Ob!Lf&P(NLbvNuRB_jzlY?n~@fKTJAUBYgF+N4W16XXpu=(Cna<@k7Oxb|2ZK z`m=c7pL~SNMD%Kay@8uOeRnsM;lUjIkyLN=phaZ&EK1gbr@~4CJ3H+~)|PZ?9yx=DgO{^G2h(fTb50?%AJh3TyGAL$~85*lRpR3>7Uzu^P^V=OpIaq-sCl!|J ztEJA0Ec*NtxL8YBsIaq|)_NSYPMBXEs^0GePo2Z}xieZ!yFahh6#!{NkC0xnBg`U} zFsA?^4+Cm^mk>&QEZxJOGmkb$_t+LcFv{>n7{^Jgs|za5n6}(oHOw@r@oNoG+>&y3 zLGGQHL>OWjXj!YoU@vVKworBZ!szA7E}En%1_oz|gA>shg(F$*7< ziOW|rIdsxu?I&hgkd>3iwJQ(JFId0vwHHh1i#_=UQ+?mhx4k$@^s9N8VQ|G})Q_d% z_1I&Nmx)Ugz0#Lj^NnvRSNg5qWLJ5JT?|HEW+8?wr6ja=b{hQBRk^+aKfkHTEz7KA z_4Rdk*FA-$id%? zI8jI4^Atsil=Evjn}uzjb~1>8#hj`Ys1ksN?9FbJsDqm3+i_ z1E)&72O6pcVeL(vv&Vam9<3v)xPBjP9tfxObfsU3(?7Z<6OTK!UP~SOC^=#+iL^N> zmRWFWOx0FsZGX{qve^djoH?eaD#|6nFMxe7zNPpsp-D8vLCR9(Si^@OU0cqIB-E_N zkrIz9QS%peDLreUUi0n=EgK$Nh>vTKMo6VD?)!rA+>ls-^IWYF&ou1j7Y|f@Cs|r) zn=Vec7>zz;Db^o6zE@<09p9H3?0RHR^bGx4A7PByb@ za6H0={`C_lqQ0*RNHkAxA7trJ*_axsBZEw6TAlH;<{H4|C0#zGF2hRnfh-Ug?bSsn z5F<*-aK7wD4mXcfoq+X|P$|g2xxO+x+imS;rX-O3sB96*)G+#HF!c61OSlfKDeKz1 zSlX&?s!CtDmXcSV7UqKGo7>pPW7G9b8Q~(3$8GGyVID(KeU!%KDEjrZg-NdXSfOx_JP%BbGMIq4RgPk zHq6Byv&7h9Ue&;$W5~fU6d{P2n6FNdJouP!WvY$0T%eOWt)@>im`$?U5-kwAt0`(q zNdmF5v7N>>sJ$Je{wx{NqdQwD8eG9!sE>Q?%A{JeDi=8ErYjwAeQomK0_|gYz8D0Pz()P^{r+7q@*lkZNL%|KEHpr%9U5a#D{jh0T2;i zvALjj4+olcR=`rLl?j5`TZFYZqO!ayiQdU0???*6=bA6wWMiaYwoWf}bu_mgi1mnZ zn*X7wrSQe)VvG^Kj_+u#xiAHzyf$>w|Dws1!%rl+zCZv14| z(IdRYK9p|KUY{AUe69Nu5ix}ucW2^T#LE@iQrW$J_$q(k4jI%Ol&+e>^RD4YFwX2F z7SZN4j(Mo^2VW!fzw#lTX`wW0gPS+g%?dI`E2uKZPP zv!Kf#OpIeLUG7LH1+#UPnlNW>)alOOs=%%|@;&~`-AQz^U#ZWMW714jo7TzGba^Jp z_UmwhWq+1@qQub!&hFrwTPrV5k$Cz=P!<8I>ziY%0doB_?ox~Dl)%VcyXoj*a9*|R zG&=T1c>PC4MVDS#WzUK5rnyg9bP63Zuch}|_95NYo-Uf`n&#!<6IzG`1f5&&)0Umb zl7Uc9DsSPG2C^U*wY8v%QIGn3>S}Vf^NI7;{%jH&$F7z{1d5WDssc7VhAAECTj&&r zrna(0v-YizMk)R#lvjZgNj|A#X+4};Pus;@q`EU4o^JcI*?fkR4wB5>d}fBuB!xBz zh?(10m5%w{U0Nsax5lh}6)X_7BvW|Net1;HKvgBB>g+JNEMI=5z*35~{FRC|NM4f9 z{d-uq!T6F`S9AH@yrRb%>06C?_YQs7h^{S{VV|3CZ6EqL(?Sc}g%wbP4usB-zA`wQ zg9{fv6QOpI?#2Z>)2#(UR*|oNN$_|l{;)82ZJ!!p9dMf!X`#?6#2V z`TTv{n#VQ&ISTsKlf2JyF1+FD|9hO;bL8%G=BlM^qW_(K=m(f!BEwwgU&5is@!uc( zp{<7Kyqt6H!ao;KyREi|?m@}ESP}YPuCR{KuUmTT^61dNC-S!4zRjN#BDg=~p9c(X qIEel)kF8O&*Z [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 is required to have in their password. + ## Whitelist email domains > [Introduced][ce-598] in GitLab 7.11.0 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7ef8b0ae2c68..7db324966d6e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -11314,6 +11314,9 @@ msgstr "" msgid "Minimum length is %{minimum_password_length} characters." msgstr "" +msgid "Minimum password length" +msgstr "" + msgid "Minutes" msgstr "" diff --git a/spec/models/concerns/devise_validatable_spec.rb b/spec/models/concerns/devise_validatable_spec.rb deleted file mode 100644 index 8ab3e10b3582..000000000000 --- a/spec/models/concerns/devise_validatable_spec.rb +++ /dev/null @@ -1,141 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe DeviseValidatable do - describe 'User' do - let_it_be(:existing_user) { create(:user) } - - context 'password' do - it 'requires password to be set for a new record' do - user = build(:user, password: '', password_confirmation: '') - - expect(user).to be_invalid - expect(user.errors[:password].join).to eq('can\'t be blank') - end - - it 'requires password_confirmation to be set for a new record' do - user = build(:user, password: 'new_password', password_confirmation: 'something_else') - - expect(user).to be_invalid - expect(user.errors[:password_confirmation].join).to eq('doesn\'t match Password') - end - - it 'requires password to be set while updating/resetting password' do - existing_user.password = '' - existing_user.password_confirmation = '' - - expect(existing_user).to be_invalid - expect(existing_user.errors[:password].join).to eq('can\'t be blank') - end - - it 'requires password_confirmation to be set while updating/resetting password' do - existing_user.password = 'set_new_password' - existing_user.password_confirmation = 'something_else' - - expect(existing_user).to be_invalid - expect(existing_user.errors[:password_confirmation].join).to eq('doesn\'t match Password') - end - - it 'does not fail validations for password length when password is not changed' do - existing_user.password = existing_user.password_confirmation = nil - - expect(existing_user).to be_valid - end - - context 'password length' do - before do - expect(User).to receive(:password_length).twice.and_return(10..128) - end - - it 'validates minimum password length' do - user = build(:user, password: 'x' * 9, password_confirmation: 'x' * 9) - - expect(user).to be_invalid - expect(user.errors[:password].join).to eq('is too short (minimum is 10 characters)') - end - - it 'validates maximum password length' do - user = build(:user, password: 'x' * 129, password_confirmation: 'x' * 129) - - expect(user).to be_invalid - expect(user.errors[:password].join).to eq('is too long (maximum is 128 characters)') - end - - it 'validates password length even if password is not required' do - user = build(:user, password: 'x' * 129, password_confirmation: 'x' * 129) - expect(user).to receive(:password_required?).twice.and_return(false) - - expect(user).to be_invalid - expect(user.errors[:password].join).to eq('is too long (maximum is 128 characters)') - end - - context 'min and max length values' do - it 'matches the values set by the `password_length` method' do - validators = User.validators_on :password - length = validators.find { |v| v.kind == :length } - - assert_equal 10, length.options[:minimum].call - assert_equal 128, length.options[:maximum].call - end - end - end - end - - context 'email' do - it 'requires email to be set' do - user = build(:user, email: nil) - - expect(user).to be_invalid - expect(user.errors[:email].join).to eq('can\'t be blank') - end - - it 'accepts valid formats' do - %w(a.b.c@example.com test_mail@gmail.com any@any.net email@test.br 123@mail.test 1☃3@mail.test).each do |email| - user = build(:user, email: email) - - expect(user).to be_valid - expect(user.errors[:email]).to be_blank - end - end - - it 'is case-senstive' do - user = build(:user, email: 'test@test.com') - - expect(user).to be_valid - - new_user = build(:user, email: 'TEST@TEST.COM') - - expect(new_user).to be_valid - expect(new_user.errors[:email].join).to be_blank - end - - context 'when email changes' do - it 'validates uniqueness, allowing blank' do - user = build(:user, email: '') - - expect(user).to be_invalid - expect(user.errors[:email].join).not_to include('taken') - - user.email = existing_user.email - - expect(user).to be_invalid - expect(user.errors[:email].join).to include('taken') - end - - it 'validates format, allowing blank' do - user = build(:user, email: '') - - expect(user).to be_invalid - expect(user.errors[:email].join).not_to include('invalid') - - %w{invalid_email_format 123 $$$ () ☃}.each do |email| - user.email = email - expect(user).to be_invalid - expect(user.errors[:email].join).to include('invalid') - end - end - end - end - end -end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fafb21a29908..570d03db3fc1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -98,6 +98,33 @@ describe User, :do_not_mock_admin_mode do end describe 'validations' do + describe 'password' do + before do + 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 + it 'has only one length validator' do + validators = described_class.validators_on :password + length_validators = validators.select { |v| v.kind == :length } + + expect(length_validators.size).to eq(1) + end + + it 'supports dynamic minimum and maximum length validation' do + validators = described_class.validators_on :password + length_validator = validators.find { |v| v.kind == :length } + + expect(length_validator.options[:minimum]).to be_a_kind_of(Proc) + expect(length_validator.options[:maximum]).to be_a_kind_of(Proc) + 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) } @@ -480,10 +507,10 @@ describe User, :do_not_mock_admin_mode do context 'maximum value' do before do - stub_const('ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH', 201) + allow(Devise.password_length).to receive(:max).and_return(201) end - it 'is determined by the current value of `ApplicationSetting::DEFAULT_MAXIMUM_PASSWORD_LENGTH`' do + it 'is determined by the current value of `Devise.password_length.max`' do expect(password_length.max).to eq(201) end end -- GitLab From cf594d81d82e8e2aa3b15aa91ceb386d9377b08f Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 6 Dec 2019 11:44:49 +0530 Subject: [PATCH 3/8] Add post-deploy migration Adds a post-deploy migration to set the value of `minimum_password length`, if the user had already set a value different than 8, using the devise initializer. --- ...05084057_update_minimum_password_length.rb | 26 ++++++++++++++++ .../update_minimum_password_length_spec.rb | 30 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 db/post_migrate/20191205084057_update_minimum_password_length.rb create mode 100644 spec/migrations/update_minimum_password_length_spec.rb diff --git a/db/post_migrate/20191205084057_update_minimum_password_length.rb b/db/post_migrate/20191205084057_update_minimum_password_length.rb new file mode 100644 index 000000000000..4e5e3dfdb864 --- /dev/null +++ b/db/post_migrate/20191205084057_update_minimum_password_length.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class UpdateMinimumPasswordLength < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + application_setting = ApplicationSetting.current_without_cache + return unless application_setting + + value_to_be_updated_to = [ + Devise.password_length.min, + ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH + ].max + + application_setting.update(minimum_password_length: value_to_be_updated_to) + end + + def down + application_setting = ApplicationSetting.current_without_cache + return unless application_setting + + value_to_be_updated_to = ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH + + application_setting.update(minimum_password_length: value_to_be_updated_to) + end +end diff --git a/spec/migrations/update_minimum_password_length_spec.rb b/spec/migrations/update_minimum_password_length_spec.rb new file mode 100644 index 000000000000..0a763e5ce0ff --- /dev/null +++ b/spec/migrations/update_minimum_password_length_spec.rb @@ -0,0 +1,30 @@ +# 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 -- GitLab From 4802b9fdf73ffa6f7adbd44525a27c9c8efc17cc Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 6 Dec 2019 17:16:19 +0530 Subject: [PATCH 4/8] Use raw SQL Update to update `minimum_password_length` --- ...191205084057_update_minimum_password_length.rb | 14 ++++++-------- doc/security/password_length_limits.md | 5 +++-- ...=> minimum_password_length_settings_v12_6.png} | Bin .../admin_area/settings/sign_up_restrictions.md | 3 ++- 4 files changed, 11 insertions(+), 11 deletions(-) rename doc/user/admin_area/img/{minimum_password_length.png => minimum_password_length_settings_v12_6.png} (100%) diff --git a/db/post_migrate/20191205084057_update_minimum_password_length.rb b/db/post_migrate/20191205084057_update_minimum_password_length.rb index 4e5e3dfdb864..d9324347075f 100644 --- a/db/post_migrate/20191205084057_update_minimum_password_length.rb +++ b/db/post_migrate/20191205084057_update_minimum_password_length.rb @@ -4,23 +4,21 @@ class UpdateMinimumPasswordLength < ActiveRecord::Migration[5.2] DOWNTIME = false def up - application_setting = ApplicationSetting.current_without_cache - return unless application_setting - value_to_be_updated_to = [ Devise.password_length.min, ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH ].max - application_setting.update(minimum_password_length: value_to_be_updated_to) + execute "UPDATE application_settings SET minimum_password_length = #{value_to_be_updated_to}" + + ApplicationSetting.expire end def down - application_setting = ApplicationSetting.current_without_cache - return unless application_setting - value_to_be_updated_to = ApplicationSetting::DEFAULT_MINIMUM_PASSWORD_LENGTH - application_setting.update(minimum_password_length: value_to_be_updated_to) + execute "UPDATE application_settings SET minimum_password_length = #{value_to_be_updated_to}" + + ApplicationSetting.expire end end diff --git a/doc/security/password_length_limits.md b/doc/security/password_length_limits.md index cd07cd94d310..235730eb8256 100644 --- a/doc/security/password_length_limits.md +++ b/doc/security/password_length_limits.md @@ -51,11 +51,12 @@ 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.png) +[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. ->**Note**: 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. +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. m>kXhFJ6@F-yAY(a!-ukm%s1ql63)B&Vm z1xUaGFaW5xOcsDEoXNd(3i5iC`)5cyaPK>KeMV~TfJbW-s7am<0ES_ur_mr1;e8Vi1a7C&K&c22`2`zDmi!?x>|Yw(c1^` zN!;MH!@iAG5-1_PGb&ZbBr@tr7I&61Zpp)5b+-T0B333R|WqYFg}EAhE0JGV^T` zJq}U~qhXE}=xO)d1g3Ojv?xD=WXS@5V$QKoFML}i-V|4D8I&u`w=A@F6(EOsYYrMV z-U!HtF=9jN_3Iqg=z7P5aw~;o?LsNI$JkQ^;Tew_#S|)$Ps7R`i@1J2s3Cs-Duzk2 z$!X{y1WQO0sH?C7RYgUhXo!vJ8ZN0m2j=M@j-8;?slu#^7p?-|FhMd|W0dnw43gx2 zPkN9GtJhR5a&Lkv2-|6tJO0WBrQNx$fF#xiOi7ADI7DL38mgfv5|kOWA$?`@ax18C z82hT3w+2^zb4>s_-OQ{K`@JrJM6|6ql&XPKW&<__DMB*A)vs}=I%7xhGwtvN^6HC8 zq=>pWn)O+yBlt-{$%$r9{A4gk{=;->4REgJuqg-0NhC+s z5KuOm1ldbDGf8yBanpLl5Y)3~1EZ=w93JD9;CWd_nmXyO)pJaZ2TF~8u8nB`qi7Cs zFbGM@Lk+xAJ(h0FyJV;}G0yeUK3e1!RW*fgo^(G4Aj$rke1lAX<6!k1_K!vQRkZ_KQlB}}q@@0rzG#>#`gxewSNwc*^GOkC+)g9(H07qzxOFwwTPU%i6mo&V(j{!WSq4F@}igIml z8e>L;^I7{lNuNmZy?k2RvU=|XN#y*G_0F$3dafhQt1YpI{c4isE-`Ig$s)JcjPm1t zM>@#59x1~lxC54S5z|Bhaf|LvGPBuYCi{(ssYT}UW>6bN+e6v8SO9!Cy=;W{boqp$ zd@v7yAWQg8>D0I(%3PK)0?kxOH0~0Tv5pRF4pots*}TQksS0upQlQj9p#f~eL9Rj74*Lh4J>1gCYTA)q%$?Eu`}Rs zMa8%BKuHl(5Z$iHhWGMi%-Xkm#n$2T;xb(jfhD}lHjJP&LjDJ76JvU8)z@^-{WaS) zI_#kAZ~}M)+k56!jA)xq+1VQ^b0_8z*|yVYf>^LtLqeKj3>n-p(3E*fi^DlKs;4V# zmIDFh59E}NarbT?1qAz&zzfH#3!$?-T{bMUv3z3)iq7Pf6r{btlZGAeHqrd{AzCS; z@sK5^{AWU~uUE=~KuP3+#2?SAxCeX4Nq{r9CBNp+H%i{x_GS8&GFz5$q282>sVdEv zqr{=+EMhMz^P;N_ki_`7!Q@-|U+SpPgjeQ%LiLphewEPViM%AtgDC!_k~Bf~@G9?V z8AMtqBRjIG4l4S7fn992Y>SAvC(Qx&kT9j`m?A~9T^MS&NhOaOsJNiQk~?Fg=u{Ka zE^SDq(PH!LB`d$V+n#W3%2UB!^;eO0w2odS`G>#&StMR!9m#4)XE$-<@lT3dT87}z zE6k8{v*1onqWWe{n>USwvLu(rx^+gsjV*uK)%eDulPtyc_d?^9Ish|Vr$V!yT-d)s zRr4l+wUcoyJ!wdf;=h0fF8~@Ro;G)WbCdrEAGJvVXoN#{{a<#}GmH%En-?zsZ#ybb zj?JVpurK73;A(BP7yz_h-1nzpN*0C#n!lo`{>M|%VAzN8KvF)d7Nq%?9&QQ)&0h*3 z`h83O8iIQiPK&f zfH6CVX*V<`-`RU03uMDZPT$f9cOjP85=1q7z%IoTp8(Xc=A93A4jZR-i>`Dsd;?^# zXz23;?Hl>^zO)b+ZhRtY3)1O)5ZA*}fphCz=KsY6Q0FyP_5h`qjLQ5$1V{@E{V9nK zuA`qG0m2(Xai(M>OFNp!sN`mjbCYlFl|NQBpdb3B#l!Nz>PrL2npOyx1}#wNV+D#F z6W(g4n4QZhhMj)lQr!vd86vH<0HR~8FHYD3tQ-Lt@Dxbqmbdj&cb=jThm9uUs9)ba zwDA0V%;=?NcnmAQ8`$J70;v#T2TbnomvS5NNfvn{ymTJ0QD;n}V=zos zM*kKN-zClSl!C3lPN=Rj?k)9lC-mGS)OB!?|3rScU~|?ig7dyGpt#hBrGYG0Zp-U@$zq}#NTh2>#-OIuVL@K zJL88My#b&(CY+Jw{u?CpMz4%P&?WI6Pr5oi9WmW(`o7x|sVv*6w-p_1)Kxy>P`;dyW#WFBCDp#k*!@%C zPsgW0X|1OAO-66}Nez!F-Q%h)B=3NC3&XwXq`n2722Hd>_dCq5-^@`Lx^o4xXZro; zK=kSXi@8s1-}zUd`q5RKeEZ6C+5$%WItcbwJ!j45Y(ZjJ0`-p|CB*}wOHZ%M+BXyS zuqd^GxN}2wYw(EJ^2Vl2#f=dlZi3-ly1KW4`t&h=&J z{aH`K^y&DUj653?ztS@wW&CSMRQo9mM8hk<9RLY(AlXnpXg(;MIPE)k-5;V#facDp z`2wu~Wzd8%0{s#y&^{OfRV<(fB9l%!#WHc1jlDqfVS%fjW0s&%ZSC87+$hV@vTkg( z+LQ1>H|cAZal<-h=u3G|ePq&fgIb%il3hm1a-00dT)c*O+h;}M+G#cwZ+jP)PGUy3 zS-}?}J(TNd;=v%2Q{5VRPc&*RQ(XL4Y3m@DBHODk) z_T+)&f`g|pid)eP#L^M`*xXR9MLFcf^Y`G1`|+5RnOo@$exaOJABj}$uiXCWU%op0 zaQcfj(g!Wu0s42tb#U7CtL*jB967Qrx4u@hvQn zR;-&;-U}D0P&`FYrW$&qH`hBo{XOcC%kx+-^vbib>DS9XSq&eS@_Sb{)2n()-W@#)~4gqC$hdd9f&sdWVl+R@nC#aSTeDZ6iIT319h%IEwbda{eZvf(q+b*gdU zg*WQVEmR|dPO!KKU#w+a#Tn-?+Y|mn$6bul?^qylwNnND0Gr&=I5}m|Ejh8B9VcbcyuRdoxQMpeHjs=9SZc&r zCt8)Bm|N!VwJ|Q=`W|2T$#27~BhzkNX{|7jJ)+cw%7y8_%8IV)%j`X^lr3D$(<}=k)yXs-5Bh^`6 zeyek4n6mpd8uuhrvz?2xNe$JLk(vgby#gq&dW9w5V0Qrt#GIOy$ebYhJ?IK|0ek=7 z9iSKSkX{hl^Ujg9VIgr3&p-@7yexempssRa1%3N~Uj(tg#PJQllbQl#fH~q$3=8HH z-+g%x6Clqk#z)*1Fkb+~n?lfR>;Wc{DfQF$4{{E7)2tv4kAy|pcg9x!a}VU*k=98Z zw!J`7)OY3}=+xWc&a109(ZE9Y9Kz?QAF?_^hq!l``6+Xvbc5gPGgGCRtXu_@-2BXa z0prUw>4BASMma^ee>A=#*XQ7*`Fssl&acB@<#{SuMSVtPZ3c-d#k_KH7>%a6cHA+P zQ-Nc;a)TuboPwo_(+Zo){fzo%xX_t*%1-+Xd{ZV`tr+mG);D|mB;=U6YY6{D&jy}q zkd*htQuB>{I=4BaBZ_^F$T3*HXDbqi>*2m$i=@$(?b`-U?Wx3W$pS!VO`q*l@%7}q zA{F^48o9)&X}+^!@vid57jI!TH|9`LuZA3!MnVUZm?DDcRvS!Hv|8UJJtrEt78D7= z4MHpNhj7nL1P(4QWp*p>Zm6tc&n*fz&be&NbhbR8C4S9|7N1pkuc@PS%RE<;D9=Fo zYq(HZc#bY=^(V6YB>efjc(+{V_}%bea+0k*`JDDCM$I6LBXeY=oX0vpR^(0s6(z*6 zoe~n0jet#TAD}0&%m>b9k})GXYuCGE+CrxL3zs^v^um55J)ArAf^gmDEtxx zDFU7jxs=YPRJocgux^6B9XquKwi;@tL4VB*Y#G*|jo?pkGBu)zSPtt%Q&WJ6a4qh` z9B7CTo`KrPLoZ=qGa}Ch1B7>_OOh64B_1QY>zeBG0;KDkHx-n8nyW_cR%=`nl2N*D zwN=)(9xT(xMbGo-nc)R;&^;^w+j)CJ|;#phRkwr>HOnE`l8B|Job3|yp-nAj(`f~tXPlPZvVo0cTKA0S>u*(knU{J0 z{?t2`bn@mKm+7~GE@URDZMd%xyF!@Y@im9fT#9A7*$2`5%>=XE?CvmW;*0*lFzHJl zEq?}tW#T!df|*$sozUq%S>uyN+4^54Stw&$hXo^a>cy6;4y?!$iCs^tX=jEv{95ng zux<&1D!0}aFf;4;fa?}L)YzeA&q2e$=cN9S6`#_zweXJ*{wfMQVpxZs7>j_IuuS@F-FJYTaui(R4Va6NZ6$zoxKx%?!PCxHOHhOF;TVVvk%Gs3r(pLY zxOj4W!`epV-9u&glciLaF~mRCQgNlwg_8Y7jd0VS_?S z*y;zn2M#yY&+Flp$SqBP7?qOLIZmyUOg%`CK)NektHRvy9}myugzZz(wnHT&e1|9u zFX>STKzbBWwMl)}Dj<6jc387|=meh};ahuYyIysqc^ocZeoYC{ zc7IIEO6l==2z$l+;&!MLe_4LMYLO6f0-k)gSg5eEXJp(JvHIl0^tbAQd$6B^CUPw5 zr?4!Ob!Lf&P(NLbvNuRB_jzlY?n~@fKTJAUBYgF+N4W16XXpu=(Cna<@k7Oxb|2ZK z`m=c7pL~SNMD%Kay@8uOeRnsM;lUjIkyLN=phaZ&EK1gbr@~4CJ3H+~)|PZ?9yx=DgO{^G2h(fTb50?%AJh3TyGAL$~85*lRpR3>7Uzu^P^V=OpIaq-sCl!|J ztEJA0Ec*NtxL8YBsIaq|)_NSYPMBXEs^0GePo2Z}xieZ!yFahh6#!{NkC0xnBg`U} zFsA?^4+Cm^mk>&QEZxJOGmkb$_t+LcFv{>n7{^Jgs|za5n6}(oHOw@r@oNoG+>&y3 zLGGQHL>OWjXj!YoU@vVKworBZ!szA7E}En%1_oz|gA>shg(F$*7< ziOW|rIdsxu?I&hgkd>3iwJQ(JFId0vwHHh1i#_=UQ+?mhx4k$@^s9N8VQ|G})Q_d% z_1I&Nmx)Ugz0#Lj^NnvRSNg5qWLJ5JT?|HEW+8?wr6ja=b{hQBRk^+aKfkHTEz7KA z_4Rdk*FA-$id%? zI8jI4^Atsil=Evjn}uzjb~1>8#hj`Ys1ksN?9FbJsDqm3+i_ z1E)&72O6pcVeL(vv&Vam9<3v)xPBjP9tfxObfsU3(?7Z<6OTK!UP~SOC^=#+iL^N> zmRWFWOx0FsZGX{qve^djoH?eaD#|6nFMxe7zNPpsp-D8vLCR9(Si^@OU0cqIB-E_N zkrIz9QS%peDLreUUi0n=EgK$Nh>vTKMo6VD?)!rA+>ls-^IWYF&ou1j7Y|f@Cs|r) zn=Vec7>zz;Db^o6zE@<09p9H3?0RHR^bGx4A7PByb@ za6H0={`C_lqQ0*RNHkAxA7trJ*_axsBZEw6TAlH;<{H4|C0#zGF2hRnfh-Ug?bSsn z5F<*-aK7wD4mXcfoq+X|P$|g2xxO+x+imS;rX-O3sB96*)G+#HF!c61OSlfKDeKz1 zSlX&?s!CtDmXcSV7UqKGo7>pPW7G9b8Q~(3$8GGyVID(KeU!%KDEjrZg-NdXSfOx_JP%BbGMIq4RgPk zHq6Byv&7h9Ue&;$W5~fU6d{P2n6FNdJouP!WvY$0T%eOWt)@>im`$?U5-kwAt0`(q zNdmF5v7N>>sJ$Je{wx{NqdQwD8eG9!sE>Q?%A{JeDi=8ErYjwAeQomK0_|gYz8D0Pz()P^{r+7q@*lkZNL%|KEHpr%9U5a#D{jh0T2;i zvALjj4+olcR=`rLl?j5`TZFYZqO!ayiQdU0???*6=bA6wWMiaYwoWf}bu_mgi1mnZ zn*X7wrSQe)VvG^Kj_+u#xiAHzyf$>w|Dws1!%rl+zCZv14| z(IdRYK9p|KUY{AUe69Nu5ix}ucW2^T#LE@iQrW$J_$q(k4jI%Ol&+e>^RD4YFwX2F z7SZN4j(Mo^2VW!fzw#lTX`wW0gPS+g%?dI`E2uKZPP zv!Kf#OpIeLUG7LH1+#UPnlNW>)alOOs=%%|@;&~`-AQz^U#ZWMW714jo7TzGba^Jp z_UmwhWq+1@qQub!&hFrwTPrV5k$Cz=P!<8I>ziY%0doB_?ox~Dl)%VcyXoj*a9*|R zG&=T1c>PC4MVDS#WzUK5rnyg9bP63Zuch}|_95NYo-Uf`n&#!<6IzG`1f5&&)0Umb zl7Uc9DsSPG2C^U*wY8v%QIGn3>S}Vf^NI7;{%jH&$F7z{1d5WDssc7VhAAECTj&&r zrna(0v-YizMk)R#lvjZgNj|A#X+4};Pus;@q`EU4o^JcI*?fkR4wB5>d}fBuB!xBz zh?(10m5%w{U0Nsax5lh}6)X_7BvW|Net1;HKvgBB>g+JNEMI=5z*35~{FRC|NM4f9 z{d-uq!T6F`S9AH@yrRb%>06C?_YQs7h^{S{VV|3CZ6EqL(?Sc}g%wbP4usB-zA`wQ zg9{fv6QOpI?#2Z>)2#(UR*|oNN$_|l{;)82ZJ!!p9dMf!X`#?6#2V z`TTv{n#VQ&ISTsKlf2JyF1+FD|9hO;bL8%G=BlM^qW_(K=m(f!BEwwgU&5is@!uc( zp{<7Kyqt6H!ao;KyREi|?m@}ESP}YPuCR{KuUmTT^61dNC-S!4zRjN#BDg=~p9c(X qIEel)kF8O&*Z Date: Mon, 16 Dec 2019 11:52:28 +0530 Subject: [PATCH 8/8] Refactor devise password length initializer --- app/models/user.rb | 12 +++--- ...vise_dynamic_password_length_validation.rb | 37 +++++++++++++++++++ ...evise_remove_password_length_validation.rb | 25 ------------- 3 files changed, 43 insertions(+), 31 deletions(-) create mode 100644 config/initializers/devise_dynamic_password_length_validation.rb delete mode 100644 config/initializers/devise_remove_password_length_validation.rb diff --git a/app/models/user.rb b/app/models/user.rb index 50297bffe95c..7bb376e6e068 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -158,8 +158,7 @@ class User < ApplicationRecord # # Validations # - # Note: devise :validatable above adds validations for :email - validates :password, length: { maximum: proc { password_length.max }, minimum: proc { password_length.min } }, allow_blank: true + # Note: devise :validatable above adds validations for :email and :password validates :name, presence: true, length: { maximum: 128 } validates :first_name, length: { maximum: 255 } validates :last_name, length: { maximum: 255 } @@ -378,6 +377,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 @@ -1752,10 +1756,6 @@ class User < ApplicationRecord def no_recent_activity? last_active_at.to_i <= MINIMUM_INACTIVE_DAYS.days.ago.to_i end - - def self.password_length - Gitlab::CurrentSettings.minimum_password_length..Devise.password_length.max - end end User.prepend_if_ee('EE::User') diff --git a/config/initializers/devise_dynamic_password_length_validation.rb b/config/initializers/devise_dynamic_password_length_validation.rb new file mode 100644 index 000000000000..e71b28bc495f --- /dev/null +++ b/config/initializers/devise_dynamic_password_length_validation.rb @@ -0,0 +1,37 @@ +# 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 diff --git a/config/initializers/devise_remove_password_length_validation.rb b/config/initializers/devise_remove_password_length_validation.rb deleted file mode 100644 index 0c0eb3e0acbf..000000000000 --- a/config/initializers/devise_remove_password_length_validation.rb +++ /dev/null @@ -1,25 +0,0 @@ -# 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 `models/user.rb` 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 - -# This can be removed as soon as https://github.com/plataformatec/devise/pull/5166 -# is merged into Devise. - -default_password_length_validator = User.validators_on(:password).find do |validator| - validator.kind == :length && - validator.options[:minimum].is_a?(Integer) && - validator.options[:maximum].is_a?(Integer) -end - -def default_password_length_validator.validate(*_) - # always return true to discard any validations using this specific validator. - true -end -- GitLab