diff --git a/app/assets/javascripts/app/controllers/_profile/password.coffee b/app/assets/javascripts/app/controllers/_profile/password.coffee index 9bde885bc..37c8cf2c1 100644 --- a/app/assets/javascripts/app/controllers/_profile/password.coffee +++ b/app/assets/javascripts/app/controllers/_profile/password.coffee @@ -56,26 +56,32 @@ class ProfilePassword extends App.ControllerSubContent data: JSON.stringify(params) processData: true success: @success + error: @error ) - success: (data) => - if data.message is 'ok' - @render() - @notify( - type: 'success' - msg: App.i18n.translateContent( 'Password changed successfully!' ) - ) - else - if data.notice - @notify - type: 'error' - msg: App.i18n.translateContent( data.notice[0], data.notice[1] ) - removeAll: true - else - @notify - type: 'error' - msg: __('The password could not be set. Please contact your administrator.') - removeAll: true - @formEnable( @$('form') ) + success: => + @render() + + @notify( + type: 'success' + msg: App.i18n.translateContent( 'Password changed successfully!' ) + ) + + error: (xhr, status, error) => + return if xhr.status != 422 + + data = xhr.responseJSON + + message = if data.notice + App.i18n.translateContent( data.notice[0], data.notice[1] ) + else + __('The password could not be set. Please contact your administrator.') + + @notify + type: 'error' + msg: message + removeAll: true + + @formEnable( @$('form') ) App.Config.set('Password', { prio: 2000, name: __('Password'), parent: '#profile', target: '#profile/password', controller: ProfilePassword, permission: ['user_preferences.password'] }, 'NavBarProfile') diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 350e864b5..9fd7f16b8 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -557,26 +557,26 @@ curl http://localhost/api/v1/users/password_change -v -u #{login}:#{password} -H def password_change # check old password - if !params[:password_old] - render json: { message: 'failed', notice: [__('Current password needed!')] }, status: :ok + if !params[:password_old] || !PasswordPolicy::MaxLength.valid?(params[:password_old]) + render json: { message: 'failed', notice: [__('Current password needed!')] }, status: :unprocessable_entity return end current_password_verified = PasswordHash.verified?(current_user.password, params[:password_old]) if !current_password_verified - render json: { message: 'failed', notice: [__('Current password is wrong!')] }, status: :ok + render json: { message: 'failed', notice: [__('Current password is wrong!')] }, status: :unprocessable_entity return end # set new password if !params[:password_new] - render json: { message: 'failed', notice: [__('Please supply your new password!')] }, status: :ok + render json: { message: 'failed', notice: [__('Please supply your new password!')] }, status: :unprocessable_entity return end result = PasswordPolicy.new(params[:password_new]) if !result.valid? - render json: { message: 'failed', notice: result.error }, status: :ok + render json: { message: 'failed', notice: result.error }, status: :unprocessable_entity return end diff --git a/app/models/user.rb b/app/models/user.rb index fb7e4b978..b9eacd295 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1181,6 +1181,11 @@ raise 'At least one user need to have admin permissions' # don't re-hash passwords return password if PasswordHash.crypted?(password) + if !PasswordPolicy::MaxLength.valid? password + errors.add :base, PasswordPolicy::MaxLength.error + return nil + end + # hash the plaintext password PasswordHash.crypt(password) end diff --git a/db/migrate/20220330092945_object_manager_update_user_password.rb b/db/migrate/20220330092945_object_manager_update_user_password.rb new file mode 100644 index 000000000..da3d4ec78 --- /dev/null +++ b/db/migrate/20220330092945_object_manager_update_user_password.rb @@ -0,0 +1,19 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +class ObjectManagerUpdateUserPassword < ActiveRecord::Migration[6.0] + def up + # return if it's a new setup + return if !Setting.exists?(name: 'system_init_done') + + UserInfo.current_user_id = 1 + + object_type = ObjectLookup.find_by(name: 'User') + attr = ObjectManager::Attribute.find_by object_lookup_id: object_type.id, name: 'password' + + # password length is capped at 1000 in PasswordPolicy::MaxLength::MAX_LENGTH + # if user copy-pastes a very long string + # this ensures that max length check is triggered preventing saving of truncated password + attr.data_option[:maxlength] = 1001 + attr.save! + end +end diff --git a/db/seeds/object_manager_attributes.rb b/db/seeds/object_manager_attributes.rb index 71ea5573a..6617c6718 100644 --- a/db/seeds/object_manager_attributes.rb +++ b/db/seeds/object_manager_attributes.rb @@ -1166,7 +1166,10 @@ ObjectManager::Attribute.add( data_type: 'input', data_option: { type: 'password', - maxlength: 100, + # password length is capped at 1000 in PasswordPolicy::MaxLength::MAX_LENGTH + # if user copy-pastes a very long string + # this ensures that max length check is triggered preventing saving of truncated password + maxlength: 1001, null: true, autocomplete: 'new-password', item_class: 'formGroup--halfSize', diff --git a/i18n/zammad.pot b/i18n/zammad.pot index baa378dc1..f211a53a7 100644 --- a/i18n/zammad.pot +++ b/i18n/zammad.pot @@ -5176,6 +5176,10 @@ msgstr "" msgid "Invalid password, it must be at least %s characters long!" msgstr "" +#: lib/password_policy/max_length.rb +msgid "Invalid password, it must be shorter than %s characters!" +msgstr "" + #: lib/password_policy/digit.rb msgid "Invalid password, it must contain at least 1 digit!" msgstr "" diff --git a/lib/auth/backend/internal.rb b/lib/auth/backend/internal.rb index a0bf06636..2aebdc60d 100644 --- a/lib/auth/backend/internal.rb +++ b/lib/auth/backend/internal.rb @@ -27,6 +27,12 @@ class Auth end def hash_matches? + # makes sure that very long strings supplied as password + # rejected early and not even tried to match to password + if !PasswordPolicy::MaxLength.valid? password + return false + end + # Because of legacy reason a special check exists and afterwards the # password will be saved in the current format. if PasswordHash.legacy?(user.password, password) diff --git a/lib/password_policy/max_length.rb b/lib/password_policy/max_length.rb new file mode 100644 index 000000000..8f459b65f --- /dev/null +++ b/lib/password_policy/max_length.rb @@ -0,0 +1,27 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +class PasswordPolicy + class MaxLength < PasswordPolicy::Backend + MAX_LENGTH = 1_000 + + def valid? + self.class.valid? @password + end + + def error + self.class.error + end + + def self.applicable? + true + end + + def self.valid?(input) + input.length <= MAX_LENGTH + end + + def self.error + [__('Invalid password, it must be shorter than %s characters!'), MAX_LENGTH] + end + end +end diff --git a/spec/db/migrate/object_manager_update_user_password_spec.rb b/spec/db/migrate/object_manager_update_user_password_spec.rb new file mode 100644 index 000000000..b8e166818 --- /dev/null +++ b/spec/db/migrate/object_manager_update_user_password_spec.rb @@ -0,0 +1,19 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe ObjectManagerUpdateUserPassword, type: :db_migration do + let(:attr) do + object_type = ObjectLookup.find_by(name: 'User') + ObjectManager::Attribute.find_by object_lookup_id: object_type.id, name: 'password' + end + + before do + attr.data_option['maxlength'] = 123 + attr.save! + end + + it 'changes maxlength' do + expect { migrate }.to change { attr.reload.data_option[:maxlength] }.from(123).to(1001) + end +end diff --git a/spec/lib/auth/backend/internal_spec.rb b/spec/lib/auth/backend/internal_spec.rb index e2d39eea5..39b129d7b 100644 --- a/spec/lib/auth/backend/internal_spec.rb +++ b/spec/lib/auth/backend/internal_spec.rb @@ -21,6 +21,30 @@ RSpec.describe Auth::Backend::Internal do end end + context 'when very long password is given' do + let(:password) { Faker::Lorem.characters(number: 1_111) } + let(:user) do + # temporary override constant to create a test user with a very long password + initial = PasswordPolicy::MaxLength::MAX_LENGTH + stub_const 'PasswordPolicy::MaxLength::MAX_LENGTH', 99_999 + user = create(:user, password: password) + stub_const 'PasswordPolicy::MaxLength::MAX_LENGTH', initial + user + end + + it 'does not try to verify it' do + allow(PasswordHash).to receive(:verified?) + + instance.valid? + + expect(PasswordHash).not_to have_received(:verified?) + end + + it 'returns false even though password is matching' do + expect(instance).not_to be_valid + end + end + context 'when given password matches stored hash' do let(:password) { user.password } diff --git a/spec/lib/password_policy/max_length_spec.rb b/spec/lib/password_policy/max_length_spec.rb new file mode 100644 index 000000000..ba2167a38 --- /dev/null +++ b/spec/lib/password_policy/max_length_spec.rb @@ -0,0 +1,45 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +require 'rails_helper' +require 'lib/password_policy/error_examples' + +RSpec.describe PasswordPolicy::MaxLength do + let(:long_string) { Faker::Lorem.characters(number: 1_111) } + + it_behaves_like 'declaring an error' + + describe '.applicable?' do + it 'returns true' do + expect(described_class).to be_applicable + end + end + + describe '#valid?' do + it 'long string is invalid' do + instance = described_class.new long_string + expect(instance).not_to be_valid + end + + it 'short string is valid' do + instance = described_class.new Faker::Lorem.sentence + expect(instance).to be_valid + end + end + + describe '#error' do + it 'includes value of MAX_LENGTH' do + instance = described_class.new(long_string) + expect(instance.error.last).to eq described_class::MAX_LENGTH + end + end + + describe '.valid?' do + it 'long string is invalid' do + expect(described_class).not_to be_valid(long_string) + end + + it 'short string is valid' do + expect(described_class).to be_valid(Faker::Lorem.sentence) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e5fcda784..253a180c5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -814,6 +814,16 @@ RSpec.describe User, type: :model do expect(user.password).not_to eq(another_user.password) end end + + context 'when saving a very long password' do + let(:long_string) { "asd1ASDasd!#{Faker::Lorem.characters(number: 1_000)}" } + + it 'marks object as invalid by adding error' do + user.update(password: long_string) + + expect(user.errors.full_messages).to eq([['Invalid password, it must be shorter than %s characters!', 1000]]) + end + end end describe '#phone' do diff --git a/spec/requests/overview_spec.rb b/spec/requests/overview_spec.rb index 16da5159e..528881683 100644 --- a/spec/requests/overview_spec.rb +++ b/spec/requests/overview_spec.rb @@ -35,7 +35,7 @@ RSpec.describe 'Overviews', type: :request do agent = create(:agent, password: 'we need a password here') - authenticated_as(agent) + authenticated_as(agent, password: 'wrong password') post '/api/v1/overviews', params: params, as: :json expect(response).to have_http_status(:unauthorized) expect(json_response).to be_a_kind_of(Hash) diff --git a/spec/requests/package_spec.rb b/spec/requests/package_spec.rb index a71c77461..0f2211e6c 100644 --- a/spec/requests/package_spec.rb +++ b/spec/requests/package_spec.rb @@ -46,7 +46,7 @@ RSpec.describe 'Packages', type: :request do it 'does packages index with inactive admin' do admin = create(:admin, active: false, password: 'we need a password here') - authenticated_as(admin) + authenticated_as(admin, password: 'wrong password') get '/api/v1/packages', as: :json expect(response).to have_http_status(:unauthorized) diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index 9aa4dbc23..421e3ec69 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -77,8 +77,8 @@ RSpec.describe 'User', type: :request do ) end - before do - configure_elasticsearch(rebuild: true) + before do |example| + configure_elasticsearch(rebuild: true) if example.metadata[:searchindex] end it 'does user create tests - no user' do @@ -1160,6 +1160,31 @@ RSpec.describe 'User', type: :request do end end end + + context 'ultra long password', authenticated_as: :user, searchindex: false do + let(:user) { create :agent, :with_valid_password } + let(:long_string) { "asd1ASDasd!#{Faker::Lorem.characters(number: 1_000)}" } + + it 'does not reach verifying when old password is too long' do + allow(PasswordHash).to receive(:verified?).and_call_original + + post '/api/v1/users/password_change', params: { password_old: long_string, password_new: long_string }, as: :json + + expect(PasswordHash).not_to have_received(:verified?).with(any_args, long_string) + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['message']).to eq('failed') + end + + it 'does not reach hashing when saving' do + allow(PasswordHash).to receive(:crypt).and_call_original + + post '/api/v1/users/password_change', params: { password_old: user.password_plain, password_new: long_string }, as: :json + + expect(PasswordHash).not_to have_received(:crypt) + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['message']).to eq('failed') + end + end end describe 'POST /api/v1/users', authenticated_as: -> { create(:admin) }, searchindex: false do diff --git a/spec/support/request.rb b/spec/support/request.rb index 738e41b01..554054355 100644 --- a/spec/support/request.rb +++ b/spec/support/request.rb @@ -67,7 +67,7 @@ module ZammadSpecSupportRequest # # @return nil def authenticated_as(user, via: :api_client, **options) - password = options[:password] || user.password.to_s + password = options[:password] || user.try(:password_plain) || user.password.to_s login = options[:login] || user.login case via