Maintenance: Improved handling of long passwords.

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'
+  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
-      if data.notice
+                __('The password could not be set. Please contact your administrator.')
+
     @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.')
+      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')

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
 

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

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

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',

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 ""

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)

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

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

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 }

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

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

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)

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)

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

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