From f63577633f2317d3b443a448a1f1f1eab563a9c8 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Fri, 28 Aug 2020 16:07:30 +0200 Subject: [PATCH] Fixes #2943 (raise default password security) + #2907 (Password strength settings are ignored when creating new customer accounts) --- .rubocop/todo.rspec.yml | 1 + .../_application_controller_form.coffee | 7 +- .../javascripts/app/controllers/signup.coffee | 5 +- app/controllers/users_controller.rb | 41 ++++-------- ...3_add_setting_enforce_special_character.rb | 32 +++++++++ db/seeds/settings.rb | 31 ++++++++- lib/mixin/has_backends.rb | 2 +- lib/password_policy.rb | 32 +++++++++ lib/password_policy/backend.rb | 20 ++++++ lib/password_policy/digit.rb | 18 +++++ lib/password_policy/length.rb | 16 +++++ lib/password_policy/special_character.rb | 18 +++++ .../upper_and_lower_case_characters.rb | 18 +++++ ..._setting_enforce_special_character_spec.rb | 15 +++++ spec/lib/auth_spec.rb | 14 ++++ spec/lib/password_policy/digit_spec.rb | 35 ++++++++++ spec/lib/password_policy/error_examples.rb | 13 ++++ spec/lib/password_policy/length_spec.rb | 52 +++++++++++++++ .../password_policy/special_character_spec.rb | 35 ++++++++++ .../upper_and_lower_case_characters_spec.rb | 45 +++++++++++++ spec/lib/password_policy_spec.rb | 66 +++++++++++++++++++ spec/requests/user_spec.rb | 47 +++++++++---- spec/system/setup/system_spec.rb | 6 +- spec/system/signup_spec.rb | 17 +++++ test/browser/aaa_getting_started_test.rb | 4 +- .../signup_password_change_and_reset_test.rb | 34 +++++----- 26 files changed, 554 insertions(+), 70 deletions(-) create mode 100644 db/migrate/20200522125253_issue_2943_add_setting_enforce_special_character.rb create mode 100644 lib/password_policy.rb create mode 100644 lib/password_policy/backend.rb create mode 100644 lib/password_policy/digit.rb create mode 100644 lib/password_policy/length.rb create mode 100644 lib/password_policy/special_character.rb create mode 100644 lib/password_policy/upper_and_lower_case_characters.rb create mode 100644 spec/db/migrate/issue_2943_add_setting_enforce_special_character_spec.rb create mode 100644 spec/lib/password_policy/digit_spec.rb create mode 100644 spec/lib/password_policy/error_examples.rb create mode 100644 spec/lib/password_policy/length_spec.rb create mode 100644 spec/lib/password_policy/special_character_spec.rb create mode 100644 spec/lib/password_policy/upper_and_lower_case_characters_spec.rb create mode 100644 spec/lib/password_policy_spec.rb create mode 100644 spec/system/signup_spec.rb diff --git a/.rubocop/todo.rspec.yml b/.rubocop/todo.rspec.yml index c03a2852d..9868a442c 100644 --- a/.rubocop/todo.rspec.yml +++ b/.rubocop/todo.rspec.yml @@ -333,6 +333,7 @@ RSpec/FilePath: - 'spec/db/migrate/issue_2608_missing_trigger_permission_spec.rb' - 'spec/db/migrate/issue_2460_fix_corrupted_twitter_ids_spec.rb' - 'spec/db/migrate/issue_2715_fix_broken_twitter_urls_spec.rb' + - 'spec/db/migrate/issue_2943_add_setting_enforce_special_character_spec.rb' - 'spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb' - 'spec/db/migrate/issue_2867_footer_header_public_link_spec.rb' - 'spec/lib/import/base_factory_spec.rb' diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 25ee8959a..522026c97 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -64,7 +64,12 @@ class App.ControllerForm extends App.Controller handler(params, attribute, @attributes, @idPrefix, @form, @) showAlert: (message) => - @form.find('.alert--danger').first().removeClass('hide').html(App.i18n.translateInline(message)) + if Array.isArray(message) + translated = App.i18n.translateInline(message[0], message.slice(1)) + else + translated = App.i18n.translateInline(message) + + @form.find('.alert--danger').first().removeClass('hide').html(translated) hideAlert: => @form.find('.alert--danger').addClass('hide').html() diff --git a/app/assets/javascripts/app/controllers/signup.coffee b/app/assets/javascripts/app/controllers/signup.coffee index fd10e3c0a..4b2762a68 100644 --- a/app/assets/javascripts/app/controllers/signup.coffee +++ b/app/assets/javascripts/app/controllers/signup.coffee @@ -75,10 +75,7 @@ class Index extends App.ControllerContent ) fail: (settings, details) => @formEnable(e) - if _.isArray(details.error) - @form.showAlert( App.i18n.translateInline( details.error[0], details.error[1] ) ) - else - @form.showAlert(details.error_human || details.error || 'Unable to create user!') + @form.showAlert(details.error_human || details.error || 'Unable to update object!') ) resend: (e) => diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a3f806f94..b06478fe3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -515,10 +515,9 @@ curl http://localhost/api/v1/users/password_reset_verify -v -u #{login}:#{passwo return end - # check password policy - result = password_policy(params[:password]) - if result != true - render json: { message: 'failed', notice: result }, status: :ok + result = PasswordPolicy.new(params[:password]) + if !result.valid? + render json: { message: 'failed', notice: result.error }, status: :ok return end @@ -583,10 +582,9 @@ curl http://localhost/api/v1/users/password_change -v -u #{login}:#{password} -H return end - # check password policy - result = password_policy(params[:password_new]) - if result != true - render json: { message: 'failed', notice: result }, status: :ok + result = PasswordPolicy.new(params[:password_new]) + if !result.valid? + render json: { message: 'failed', notice: result.error }, status: :ok return end @@ -888,20 +886,6 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content private - def password_policy(password) - if Setting.get('password_min_size').to_i > password.length - return ['Invalid password, it must be at least %s characters long!', Setting.get('password_min_size')] - end - if Setting.get('password_need_digit').to_i == 1 && password !~ /\d/ - return ['Invalid password, it must contain at least 1 digit!'] - end - if Setting.get('password_min_2_lower_2_upper_characters').to_i == 1 && ( password !~ /[A-Z].*[A-Z]/ || password !~ /[a-z].*[a-z]/ ) - return ['Invalid password, it must contain at least 2 lowercase and 2 uppercase characters!'] - end - - true - end - def clean_user_params User.param_cleanup(User.association_name_to_id_convert(params), true) end @@ -982,9 +966,10 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content email_taken_by = User.find_by email: clean_user_params[:email].downcase.strip - result = (password = clean_user_params[:password]) && password_policy(password) - raise Exceptions::UnprocessableEntity, 'Only signup with a password!' if result.nil? - raise Exceptions::UnprocessableEntity, result if result != true + result = PasswordPolicy.new(clean_user_params[:password]) + if !result.valid? + raise Exceptions::UnprocessableEntity, result.error + end user = User.new(clean_user_params) user.associations_from_param(params) @@ -1039,8 +1024,10 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content end # check password policy - result = (password = clean_user_params[:password]) && password_policy(password) - raise Exceptions::UnprocessableEntity, result if result != true + result = PasswordPolicy.new(clean_user_params[:password]) + if !result.valid? + raise Exceptions::UnprocessableEntity, result.error + end user = User.new(clean_user_params) user.associations_from_param(params) diff --git a/db/migrate/20200522125253_issue_2943_add_setting_enforce_special_character.rb b/db/migrate/20200522125253_issue_2943_add_setting_enforce_special_character.rb new file mode 100644 index 000000000..e251f8601 --- /dev/null +++ b/db/migrate/20200522125253_issue_2943_add_setting_enforce_special_character.rb @@ -0,0 +1,32 @@ +class Issue2943AddSettingEnforceSpecialCharacter < ActiveRecord::Migration[5.2] + def up + # return if it's a new setup + return if !Setting.exists?(name: 'system_init_done') + + Setting.create_if_not_exists( + title: 'Special character required', + name: 'password_need_special_character', + area: 'Security::Password', + description: 'Password needs to contain at least one special character.', + options: { + form: [ + { + display: 'Needed', + null: true, + name: 'password_need_special_character', + tag: 'select', + options: { + 1 => 'yes', + 0 => 'no', + }, + }, + ], + }, + state: 0, + preferences: { + permission: ['admin.security'], + }, + frontend: false + ) + end +end diff --git a/db/seeds/settings.rb b/db/seeds/settings.rb index 03bc77e4f..57eefad55 100644 --- a/db/seeds/settings.rb +++ b/db/seeds/settings.rb @@ -1773,7 +1773,7 @@ Setting.create_if_not_exists( }, ], }, - state: 6, + state: 10, preferences: { permission: ['admin.security'], }, @@ -1798,7 +1798,7 @@ Setting.create_if_not_exists( }, ], }, - state: 0, + state: 1, preferences: { permission: ['admin.security'], }, @@ -1829,6 +1829,31 @@ Setting.create_if_not_exists( }, frontend: false ) +Setting.create_if_not_exists( + title: 'Special character required', + name: 'password_need_special_character', + area: 'Security::Password', + description: 'Password needs to contain at least one special character.', + options: { + form: [ + { + display: 'Needed', + null: true, + name: 'password_need_special_character', + tag: 'select', + options: { + 1 => 'yes', + 0 => 'no', + }, + }, + ], + }, + state: 0, + preferences: { + permission: ['admin.security'], + }, + frontend: false +) Setting.create_if_not_exists( title: 'Maximum failed logins', name: 'password_max_login_failed', @@ -1862,7 +1887,7 @@ Setting.create_if_not_exists( }, ], }, - state: 10, + state: 5, preferences: { permission: ['admin.security'], }, diff --git a/lib/mixin/has_backends.rb b/lib/mixin/has_backends.rb index 57beae99f..5bad7581f 100644 --- a/lib/mixin/has_backends.rb +++ b/lib/mixin/has_backends.rb @@ -3,7 +3,7 @@ module Mixin extend ActiveSupport::Concern included do - cattr_accessor :backends do + class_attribute :backends do Set.new end diff --git a/lib/password_policy.rb b/lib/password_policy.rb new file mode 100644 index 000000000..5980c340e --- /dev/null +++ b/lib/password_policy.rb @@ -0,0 +1,32 @@ +# Check if password matches system settings +class PasswordPolicy + include ::Mixin::HasBackends + + attr_reader :password + + # @param password [String, nil] to evaluate. nil is treated as empty string + def initialize(password) + @password = password || '' + end + + def valid? + errors.blank? + end + + def error + errors.first + end + + def errors + @errors ||= applicable_backends + .map { |backend| backend.new(password) } + .reject(&:valid?) + .map(&:error) + end + + private + + def applicable_backends + @applicable_backends ||= backends.select(&:applicable?) + end +end diff --git a/lib/password_policy/backend.rb b/lib/password_policy/backend.rb new file mode 100644 index 000000000..0baa52881 --- /dev/null +++ b/lib/password_policy/backend.rb @@ -0,0 +1,20 @@ +class PasswordPolicy + class Backend + # @param password [String] to evaluate + def initialize(password) + @password = password + end + + def valid? + false + end + + def error + ['Unknown error'] + end + + def self.applicable? + false + end + end +end diff --git a/lib/password_policy/digit.rb b/lib/password_policy/digit.rb new file mode 100644 index 000000000..4ceebd5f3 --- /dev/null +++ b/lib/password_policy/digit.rb @@ -0,0 +1,18 @@ +class PasswordPolicy + class Digit < PasswordPolicy::Backend + + NEED_DIGIT_REGEXP = /\d/.freeze + + def valid? + @password.match? NEED_DIGIT_REGEXP + end + + def error + ['Invalid password, it must contain at least 1 digit!'] + end + + def self.applicable? + Setting.get('password_need_digit').to_i == 1 + end + end +end diff --git a/lib/password_policy/length.rb b/lib/password_policy/length.rb new file mode 100644 index 000000000..1764e2d79 --- /dev/null +++ b/lib/password_policy/length.rb @@ -0,0 +1,16 @@ +class PasswordPolicy + class Length < PasswordPolicy::Backend + + def valid? + Setting.get('password_min_size').to_i <= @password.length + end + + def error + ['Invalid password, it must be at least %s characters long!', Setting.get('password_min_size')] + end + + def self.applicable? + true + end + end +end diff --git a/lib/password_policy/special_character.rb b/lib/password_policy/special_character.rb new file mode 100644 index 000000000..d1a065412 --- /dev/null +++ b/lib/password_policy/special_character.rb @@ -0,0 +1,18 @@ +class PasswordPolicy + class SpecialCharacter < PasswordPolicy::Backend + + NEED_SPECIAL_CHARACTER_REGEXP = /\W/.freeze + + def valid? + @password.match? NEED_SPECIAL_CHARACTER_REGEXP + end + + def error + ['Invalid password, it must contain at least 1 special character!'] + end + + def self.applicable? + Setting.get('password_need_special_character').to_i == 1 + end + end +end diff --git a/lib/password_policy/upper_and_lower_case_characters.rb b/lib/password_policy/upper_and_lower_case_characters.rb new file mode 100644 index 000000000..95268b49f --- /dev/null +++ b/lib/password_policy/upper_and_lower_case_characters.rb @@ -0,0 +1,18 @@ +class PasswordPolicy + class UpperAndLowerCaseCharacters < PasswordPolicy::Backend + + UPPER_LOWER_REGEXPS = [/\p{Upper}.*\p{Upper}/, /\p{Lower}.*\p{Lower}/].freeze + + def valid? + UPPER_LOWER_REGEXPS.all? { |regexp| @password.match?(regexp) } + end + + def error + ['Invalid password, it must contain at least 2 lowercase and 2 uppercase characters!'] + end + + def self.applicable? + Setting.get('password_min_2_lower_2_upper_characters').to_i == 1 + end + end +end diff --git a/spec/db/migrate/issue_2943_add_setting_enforce_special_character_spec.rb b/spec/db/migrate/issue_2943_add_setting_enforce_special_character_spec.rb new file mode 100644 index 000000000..c8094a367 --- /dev/null +++ b/spec/db/migrate/issue_2943_add_setting_enforce_special_character_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +RSpec.describe Issue2943AddSettingEnforceSpecialCharacter, type: :db_migration do + before do + Setting.find_by(name: :password_need_special_character).destroy + end + + it 'adds password_need_special_character setting' do + expect { migrate }.to change { Setting.exists?(name: :password_need_special_character) }.from(false).to(true) + end + + it 'performs no action for new systems', system_init_done: false do + expect { migrate }.not_to change { Setting.exists?(name: :password_need_special_character) }.from(false) + end +end diff --git a/spec/lib/auth_spec.rb b/spec/lib/auth_spec.rb index 9a221bcd3..43c807e84 100644 --- a/spec/lib/auth_spec.rb +++ b/spec/lib/auth_spec.rb @@ -32,6 +32,20 @@ RSpec.describe Auth do expect(result).to be false end end + + context 'given default password_max_login_failed' do + it 'passes with 5 attempts' do + user = create(:user, login_failed: 5) + result = described_class.can_login?(user) + expect(result).to be true + end + + it 'fails with 6 attempts' do + user = create(:user, login_failed: 6) + result = described_class.can_login?(user) + expect(result).to be false + end + end end describe '.valid?' do diff --git a/spec/lib/password_policy/digit_spec.rb b/spec/lib/password_policy/digit_spec.rb new file mode 100644 index 000000000..17df16dd3 --- /dev/null +++ b/spec/lib/password_policy/digit_spec.rb @@ -0,0 +1,35 @@ +require 'rails_helper' +require 'lib/password_policy/error_examples' + +RSpec.describe PasswordPolicy::Digit do + it_behaves_like 'declaring an error' + + describe '.applicable?' do + it "returns false when Setting 'password_need_digit' is disabled" do + Setting.set('password_need_digit', 0) + expect(described_class).not_to be_applicable + end + + it "returns true when Setting 'password_need_digit' is enabled" do + Setting.set('password_need_digit', 1) + expect(described_class).to be_applicable + end + end + + describe '#valid?' do + it 'valid when digit is included' do + instance = described_class.new('goodPW!!1111') + expect(instance).to be_valid + end + + it 'not valid when only letters' do + instance = described_class.new('notsogoodpw') + expect(instance).not_to be_valid + end + + it 'not valid when includes special characters' do + instance = described_class.new('notsogoodpw@#!') + expect(instance).not_to be_valid + end + end +end diff --git a/spec/lib/password_policy/error_examples.rb b/spec/lib/password_policy/error_examples.rb new file mode 100644 index 000000000..4e76090fc --- /dev/null +++ b/spec/lib/password_policy/error_examples.rb @@ -0,0 +1,13 @@ +RSpec.shared_examples 'declaring an error' do + describe '#error' do + let(:instance) { described_class.new('') } + + it 'returns an Array' do + expect(instance.error).to be_kind_of(Array) + end + + it 'is contains at least one value' do + expect(instance.error).to be_present + end + end +end diff --git a/spec/lib/password_policy/length_spec.rb b/spec/lib/password_policy/length_spec.rb new file mode 100644 index 000000000..abd52a03d --- /dev/null +++ b/spec/lib/password_policy/length_spec.rb @@ -0,0 +1,52 @@ +require 'rails_helper' +require 'lib/password_policy/error_examples' + +RSpec.describe PasswordPolicy::Length do + it_behaves_like 'declaring an error' + + describe '.applicable?' do + it "returns true when Setting 'password_min_size' is zero" do + Setting.set('password_min_size', 0) + expect(described_class).to be_applicable + end + + it "returns true when Setting 'password_min_size' is 10" do + Setting.set('password_min_size', 10) + expect(described_class).to be_applicable + end + end + + describe '#valid?' do + it "valid when password is longer than Setting 'password_min_size'" do + Setting.set('password_min_size', 2) + instance = described_class.new('good') + expect(instance).to be_valid + end + + it "not valid when password is shorter than Setting 'password_min_size'" do + Setting.set('password_min_size', 2) + instance = described_class.new('g') + expect(instance).not_to be_valid + end + + it "valid when password is exactly Setting 'password_min_size'" do + Setting.set('password_min_size', 4) + instance = described_class.new('good') + expect(instance).to be_valid + end + + it "valid when Setting 'password_min_size' is zero" do + Setting.set('password_min_size', 0) + instance = described_class.new('good') + expect(instance).to be_valid + end + end + + describe 'error' do + it "includes value of Setting 'password_min_size'" do + Setting.set('password_min_size', 123) + instance = described_class.new('') + expect(instance.error.last).to be(123) + end + end +end diff --git a/spec/lib/password_policy/special_character_spec.rb b/spec/lib/password_policy/special_character_spec.rb new file mode 100644 index 000000000..31531b02e --- /dev/null +++ b/spec/lib/password_policy/special_character_spec.rb @@ -0,0 +1,35 @@ +require 'rails_helper' +require 'lib/password_policy/error_examples' + +RSpec.describe PasswordPolicy::SpecialCharacter do + it_behaves_like 'declaring an error' + + describe '.applicable?' do + it "returns false when Setting 'password_need_special_character' is disabled" do + Setting.set('password_need_special_character', 0) + expect(described_class).not_to be_applicable + end + + it "returns true when Setting 'password_need_digit' is enabled" do + Setting.set('password_need_special_character', 1) + expect(described_class).to be_applicable + end + end + + describe '#valid?' do + it 'valid when special character is included' do + instance = described_class.new('gütenTag') + expect(instance).to be_valid + end + + it 'not valid when only letters' do + instance = described_class.new('notsogoodpw') + expect(instance).not_to be_valid + end + + it 'not valid when includes digit' do + instance = described_class.new('notsogoodpw123') + expect(instance).not_to be_valid + end + end +end diff --git a/spec/lib/password_policy/upper_and_lower_case_characters_spec.rb b/spec/lib/password_policy/upper_and_lower_case_characters_spec.rb new file mode 100644 index 000000000..e0e4b6f0c --- /dev/null +++ b/spec/lib/password_policy/upper_and_lower_case_characters_spec.rb @@ -0,0 +1,45 @@ +require 'rails_helper' +require 'lib/password_policy/error_examples' + +RSpec.describe PasswordPolicy::UpperAndLowerCaseCharacters do + it_behaves_like 'declaring an error' + + describe '.applicable?' do + it "returns false when Setting 'password_min_2_lower_2_upper_characters' is disabled" do + Setting.set('password_min_2_lower_2_upper_characters', 0) + expect(described_class).not_to be_applicable + end + + it "returns true when Setting 'password_min_2_lower_2_upper_characters' is enabled" do + Setting.set('password_min_2_lower_2_upper_characters', 1) + expect(described_class).to be_applicable + end + end + + describe '#valid?' do + it 'valid when upper and lower letters included' do + instance = described_class.new('abcDE') + expect(instance).to be_valid + end + + it 'valid when upper and lower letters included are non-ASCII' do + instance = described_class.new('ąčŪŽ') + expect(instance).to be_valid + end + + it 'not valid when only upper letters' do + instance = described_class.new('NOTSOGOODPW') + expect(instance).not_to be_valid + end + + it 'not valid when only lower letters' do + instance = described_class.new('notsogoodpw') + expect(instance).not_to be_valid + end + + it 'not valid when only upper letter included' do + instance = described_class.new('abcD') + expect(instance).not_to be_valid + end + end +end diff --git a/spec/lib/password_policy_spec.rb b/spec/lib/password_policy_spec.rb new file mode 100644 index 000000000..786cff07c --- /dev/null +++ b/spec/lib/password_policy_spec.rb @@ -0,0 +1,66 @@ +require 'rails_helper' + +RSpec.describe PasswordPolicy do + + subject(:instance) { described_class.new('some_password') } + + let(:passing_job_class) { make_job_class(valid: true, applicable: true, error: 'passing job') } + let(:failing_job_class) { make_job_class(valid: false, applicable: true, error: 'failing job') } + let(:another_failing_job_class) { make_job_class(valid: false, applicable: true, error: 'another failing job') } + let(:not_applicable_job_class) { make_job_class(valid: false, applicable: false, error: 'not applicable job') } + + describe '#valid?' do + it 'returns true when all backends pass' do + instance.backends = Set.new [passing_job_class] + expect(instance).to be_valid + end + + it 'returns false when one of backends fail' do + instance.backends = Set.new [passing_job_class, failing_job_class] + expect(instance).not_to be_valid + end + + it 'returns true when one of backends fail but is not applicable' do + instance.backends = Set.new [passing_job_class, not_applicable_job_class] + expect(instance).to be_valid + end + end + + describe '#error' do + it 'returns nil when no errors present' do + instance.backends = Set.new + expect(instance.error).to be_nil + end + + it 'returns Array with the error when an error is present' do + instance.backends = Set.new [failing_job_class] + expect(instance.errors.first).to eq(['failing job']) + end + + it 'returns Array with the first error when multiple errors are present' do + instance.backends = Set.new [another_failing_job_class, failing_job_class] + expect(instance.errors.first).to eq(['another failing job']) + end + end + + describe '#errors' do + it 'returns empty Array when all backends pass' do + instance.backends = Set.new [passing_job_class] + expect(instance.errors).to be_kind_of(Array).and(be_blank) + end + + it 'returns an Array of Arrays when one of backends fail' do + instance.backends = Set.new [failing_job_class] + expect(instance.errors.first).to be_kind_of(Array) + end + end + + def make_job_class(valid:, applicable:, error:) + klass = Class.new(PasswordPolicy::Backend) + klass.define_method(:valid?) { valid } + klass.define_singleton_method(:applicable?) { applicable } + klass.define_method(:error) { [error] } + + klass + end +end diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index 19cc3183c..a44fbc4d0 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -97,10 +97,9 @@ RSpec.describe 'User', type: :request do post '/api/v1/users', params: params, headers: headers, as: :json expect(response).to have_http_status(:unprocessable_entity) expect(json_response['error']).to be_truthy - expect(json_response['error']).to eq('Only signup with a password!') # already existing user with enabled feature, pretend signup is successful - params = { email: 'rest-customer1@example.com', password: 'asd1ASD', signup: true } + params = { email: 'rest-customer1@example.com', password: 'asd1ASDasd!', signup: true } post '/api/v1/users', params: params, headers: headers, as: :json expect(response).to have_http_status(:created) expect(json_response).to be_truthy @@ -120,7 +119,7 @@ RSpec.describe 'User', type: :request do expect(json_response['error']).to eq('Attribute \'email\' required!') # create user with enabled feature (take customer role) - params = { firstname: 'Me First', lastname: 'Me Last', password: 'asd1ASD', email: 'new_here@example.com', signup: true } + params = { firstname: 'Me First', lastname: 'Me Last', email: 'new_here@example.com', password: '1asdASDasd', signup: true } post '/api/v1/users', params: params, headers: headers, as: :json expect(response).to have_http_status(:created) expect(json_response).to be_truthy @@ -133,7 +132,7 @@ RSpec.describe 'User', type: :request do # create user with admin role (not allowed for signup, take customer role) role = Role.lookup(name: 'Admin') - params = { firstname: 'Admin First', lastname: 'Admin Last', email: 'new_admin@example.com', password: 'asd1ASD', role_ids: [ role.id ], signup: true } + params = { firstname: 'Admin First', lastname: 'Admin Last', email: 'new_admin@example.com', role_ids: [ role.id ], signup: true, password: '1asdASDasd' } post '/api/v1/users', params: params, headers: headers, as: :json expect(response).to have_http_status(:created) expect(json_response).to be_truthy @@ -144,7 +143,7 @@ RSpec.describe 'User', type: :request do # create user with agent role (not allowed for signup, take customer role) role = Role.lookup(name: 'Agent') - params = { firstname: 'Agent First', lastname: 'Agent Last', email: 'new_agent@example.com', password: 'asd1ASD', role_ids: [ role.id ], signup: true } + params = { firstname: 'Agent First', lastname: 'Agent Last', email: 'new_agent@example.com', role_ids: [ role.id ], signup: true, password: '1asdASDasd' } post '/api/v1/users', params: params, headers: headers, as: :json expect(response).to have_http_status(:created) expect(json_response).to be_truthy @@ -164,6 +163,22 @@ RSpec.describe 'User', type: :request do expect(json_response['error']).to eq('authentication failed') end + context 'password security' do + it 'verified with no current user' do + params = { email: 'some_new_customer@example.com', password: 'asdasdasdasd', signup: true } + post '/api/v1/users', params: params, headers: headers, as: :json + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['error']).to be_truthy + expect(json_response['error']).to include('Invalid password') + end + + it 'verified with no current user', authenticated_as: :admin do + params = { email: 'some_new_customer@example.com', password: 'asd' } + post '/api/v1/users', params: params, headers: headers, as: :json + expect(response).to have_http_status(:created) + end + end + it 'does auth tests - not existing user' do authenticated_as(nil, login: 'not_existing@example.com', password: 'adminpw') get '/api/v1/users/me', params: {}, as: :json @@ -1078,7 +1093,7 @@ RSpec.describe 'User', type: :request do context 'for user with email address' do it 'return ok' do - post '/api/v1/users/password_reset_verify', params: { username: user.login, token: token.name, password: 'Test1234#.' }, as: :json + post '/api/v1/users/password_reset_verify', params: { username: user.login, token: token.name, password: 'TEst1234#.' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) @@ -1108,7 +1123,7 @@ RSpec.describe 'User', type: :request do let(:user) { create(:customer, login: 'somebody', email: '', password: 'Test1234#.') } it 'return ok' do - post '/api/v1/users/password_change', params: { password_old: 'Test1234#.', password_new: 'Test12345#.' }, as: :json + post '/api/v1/users/password_change', params: { password_old: 'Test1234#.', password_new: 'TEst12345#.' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) @@ -1118,7 +1133,7 @@ RSpec.describe 'User', type: :request do context 'user with email address' do it 'return ok' do - post '/api/v1/users/password_change', params: { password_old: 'Test1234#.', password_new: 'Test12345#.' }, as: :json + post '/api/v1/users/password_change', params: { password_old: 'Test1234#.', password_new: 'TEst12345#.' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) @@ -1133,7 +1148,7 @@ RSpec.describe 'User', type: :request do post '/api/v1/users', params: params, as: :json end - let(:successful_params) { { email: 'non_existant@example.com' } } + let(:successful_params) { { email: attributes_for(:admin)[:email] } } let(:params_with_role) { successful_params.merge({ role_ids: [Role.find_by(name: 'Admin').id] } ) } let(:params_with_invite) { successful_params.merge({ invite: true } ) } @@ -1215,7 +1230,7 @@ RSpec.describe 'User', type: :request do end end - describe 'POST /api/v1/users processed by #create_admin' do + describe 'POST /api/v1/users processed by #create_admin', authenticated_as: false do before do User.all[2...].each(&:destroy) # destroy previously created users end @@ -1224,7 +1239,11 @@ RSpec.describe 'User', type: :request do post '/api/v1/users', params: params, as: :json end - let(:successful_params) { { firstname: 'Admin First', lastname: 'Admin Last', email: 'new_admin@example.com', password: 'asd1ASD' } } + let(:successful_params) do + email = attributes_for(:admin)[:email] + + { firstname: 'Admin First', lastname: 'Admin Last', email: email, password: 'asd1ASDasd!' } + end it 'succeds' do make_request successful_params @@ -1276,7 +1295,11 @@ RSpec.describe 'User', type: :request do post '/api/v1/users', params: params, as: :json end - let(:successful_params) { { firstname: 'Customer First', lastname: 'Customer Last', email: 'new_customer@example.com', password: 'asd1ASD', signup: true } } + let(:successful_params) do + email = attributes_for(:admin)[:email] + + { firstname: 'Customer First', lastname: 'Customer Last', email: email, password: 'gsd1ASDasd!', signup: true } + end before do create(:admin) # simulate functional system with admin created diff --git a/spec/system/setup/system_spec.rb b/spec/system/setup/system_spec.rb index bd26bc1c8..b0568219f 100644 --- a/spec/system/setup/system_spec.rb +++ b/spec/system/setup/system_spec.rb @@ -31,8 +31,8 @@ RSpec.describe 'System setup process', type: :system, set_up: false, authenticat fill_in 'firstname', with: 'Test Master' fill_in 'lastname', with: 'Agent' fill_in 'email', with: 'master@example.com' - fill_in 'password', with: 'test1234äöüß' - fill_in 'password_confirm', with: 'test1234äöüß' + fill_in 'password', with: 'TEst1234äöüß' + fill_in 'password_confirm', with: 'TEst1234äöüß' click_on('Create') end @@ -126,7 +126,7 @@ RSpec.describe 'System setup process', type: :system, set_up: false, authenticat click_on('Create') - expect(page).to have_text 'Invalid password,' + expect(page).to have_text 'Invalid password' end end end diff --git a/spec/system/signup_spec.rb b/spec/system/signup_spec.rb new file mode 100644 index 000000000..f9c805174 --- /dev/null +++ b/spec/system/signup_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +RSpec.describe 'Signup', type: :system, authenticated_as: false do + it 'shows password strength error' do + visit 'signup' + + fill_in 'firstname', with: 'Test' + fill_in 'lastname', with: 'Test' + fill_in 'email', with: 'test@example.com' + fill_in 'password', with: 'badpw' + fill_in 'password_confirm', with: 'badpw' + + click '.js-submit' + + expect(page).to have_text 'Invalid password,' + end +end diff --git a/test/browser/aaa_getting_started_test.rb b/test/browser/aaa_getting_started_test.rb index 04566793a..4dfff0cd9 100644 --- a/test/browser/aaa_getting_started_test.rb +++ b/test/browser/aaa_getting_started_test.rb @@ -39,11 +39,11 @@ class AaaGettingStartedTest < TestCase ) set( css: '.js-admin input[name="password"]', - value: 'test1234äöüß', + value: 'TEst1234äöüß', ) set( css: '.js-admin input[name="password_confirm"]', - value: 'test1234äöüß', + value: 'TEst1234äöüß', ) click(css: '.js-admin .btn--success') diff --git a/test/browser/signup_password_change_and_reset_test.rb b/test/browser/signup_password_change_and_reset_test.rb index aa868db56..67ea1ef72 100644 --- a/test/browser/signup_password_change_and_reset_test.rb +++ b/test/browser/signup_password_change_and_reset_test.rb @@ -23,11 +23,11 @@ class SignupPasswordChangeAndResetTest < TestCase ) set( css: 'input[name="password"]', - value: 'some-pass-123', + value: 'SOme-pass1', ) set( css: 'input[name="password_confirm"]', - value: 'some-pass-123', + value: 'SOme-pass1', ) click(css: 'button.js-submit') @@ -70,7 +70,7 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password_old"]', - value: 'some-pass-123', + value: 'SOme-pass1', ) set( css: 'input[name="password_new_confirm"]', @@ -84,11 +84,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password_new"]', - value: 'some', + value: 'SOme-1', ) set( css: 'input[name="password_new_confirm"]', - value: 'some', + value: 'SOme-1', ) click(css: '.content .btn--primary') @@ -99,11 +99,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password_new"]', - value: 'some-pass-new', + value: 'SOme-pass-new', ) set( css: 'input[name="password_new_confirm"]', - value: 'some-pass-new', + value: 'SOme-pass-new', ) click(css: '.content .btn--primary') @@ -114,11 +114,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password_new"]', - value: 'some-pass-new2', + value: 'SOme-pass-new2', ) set( css: 'input[name="password_new_confirm"]', - value: 'some-pass-new2', + value: 'SOme-pass-new2', ) click(css: '.content .btn--primary') @@ -131,7 +131,7 @@ class SignupPasswordChangeAndResetTest < TestCase # check login with new pw login( username: signup_user_email, - password: 'some-pass-new2', + password: 'SOme-pass-new2', ) logout() @@ -146,7 +146,7 @@ class SignupPasswordChangeAndResetTest < TestCase # reset password (with valid session - should not be possible) login( username: signup_user_email, - password: 'some-pass-new2', + password: 'SOme-pass-new2', url: browser_url, ) @@ -207,11 +207,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password"]', - value: 'some', + value: 'SOme-1', ) set( css: 'input[name="password_confirm"]', - value: 'some', + value: 'SOme-1', ) click(css: '.content .btn--primary') watch_for( @@ -221,11 +221,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password"]', - value: 'some-pass-new', + value: 'SOme-pass-new', ) set( css: 'input[name="password_confirm"]', - value: 'some-pass-new', + value: 'SOme-pass-new', ) click(css: '.content .btn--primary') watch_for( @@ -235,11 +235,11 @@ class SignupPasswordChangeAndResetTest < TestCase set( css: 'input[name="password"]', - value: 'some-pass-new2', + value: 'SOme-pass-new2', ) set( css: 'input[name="password_confirm"]', - value: 'some-pass-new2', + value: 'SOme-pass-new2', ) click(css: '.content .btn--primary') watch_for(