From 2d22930bdb3255c90f346979dc7b6af3b918eb3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Wed, 14 Apr 2021 14:54:53 +0200 Subject: [PATCH] [36746] Unset plain password accessor after saving (#9157) * [36746] Unset plain password accessor after saving Otherwise, this password remains and is validated again due to "user.register!" which then fails with an exception https://community.openproject.org/wp/36746 * Don't try to use user.password after saving it now gets cleared * Remove user.password usage in specs The user password attribute is an accessor that now gets deleted after saving so tests can no longer depend on it being present after created from a Factory * Also retain user password in mail handler --- app/controllers/users_controller.rb | 4 ++-- app/models/mail_handler.rb | 7 ++++--- app/models/user.rb | 5 +++++ .../remember_cookie/login_with_remember_cookie_spec.rb | 9 ++++----- spec/features/auth/login_spec.rb | 4 ++-- spec/features/users/self_registration_spec.rb | 5 +++-- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 08cb7bac79..5b5b73a3ea 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -136,7 +136,7 @@ class UsersController < ApplicationController self_notified: params[:self_notified] == '1', notified_project_ids: params[:notified_project_ids]) - if !@user.password.blank? && @user.change_password_allowed? + if update_params[:password].present? && @user.change_password_allowed? send_information = params[:send_information] if @user.invited? @@ -152,7 +152,7 @@ class UsersController < ApplicationController end if @user.active? && send_information - UserMailer.account_information(@user, @user.password).deliver_later + UserMailer.account_information(@user, update_params[:password]).deliver_later end end diff --git a/app/models/mail_handler.rb b/app/models/mail_handler.rb index cc77349acb..222bb8a515 100644 --- a/app/models/mail_handler.rb +++ b/app/models/mail_handler.rb @@ -98,10 +98,10 @@ class MailHandler < ActionMailer::Base when 'accept' @user = User.anonymous when 'create' - @user = MailHandler.create_user_from_email(email) + @user, password = MailHandler.create_user_from_email(email) if @user log "[#{@user.login}] account created" - UserMailer.account_information(@user, @user.password).deliver_later + UserMailer.account_information(@user, password).deliver_later else log "could not create account for [#{sender_email}]", :error return false @@ -442,8 +442,9 @@ class MailHandler < ActionMailer::Base end if addr.present? user = new_user_from_attributes(addr, name) + password = user.password if user.save - user + [user, password] else log "failed to create User: #{user.errors.full_messages}", :error nil diff --git a/app/models/user.rb b/app/models/user.rb index 4633fdfca3..ea3e46a67d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -166,6 +166,7 @@ class User < Principal passwords.reload clean_up_former_passwords + clean_up_password_attribute end end @@ -731,6 +732,10 @@ class User < Principal (passwords[keep_count..-1] || []).each(&:destroy) end + def clean_up_password_attribute + self.password = self.password_confirmation = nil + end + ## # Brute force prevention - class methods # diff --git a/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb b/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb index 0c748486ea..641fed0a40 100644 --- a/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb +++ b/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb @@ -6,12 +6,11 @@ describe 'Login with 2FA remember cookie', with_2fa_ee: true, with_config: { '2fa': { active_strategies: [:developer], allow_remember_for_days: 30 } }, js: true do - let(:user) do - FactoryBot.create(:user) - end let(:user_password) do - # Works because the user is not reloaded - user.password + "user!user!" + end + let(:user) do + FactoryBot.create(:user, password: user_password, password_confirmation: user_password) end let!(:device) { FactoryBot.create :two_factor_authentication_device_sms, user: user, active: true, default: true } diff --git a/spec/features/auth/login_spec.rb b/spec/features/auth/login_spec.rb index 2987b0ddc9..f3affef1d0 100644 --- a/spec/features/auth/login_spec.rb +++ b/spec/features/auth/login_spec.rb @@ -68,7 +68,7 @@ describe 'Login', type: :feature do context 'with leading and trailing space in login' do it 'can still login' do # first login - login_with(" #{user.login} ", user.password) + login_with(" #{user.login} ", user_password) # on the my page expect(page) @@ -83,7 +83,7 @@ describe 'Login', type: :feature do it 'redirects to homescreen after forced password change (with validation error) and first login' do # first login - login_with(user.login, user.password) + login_with(user.login, user_password) expect(current_path).to eql signin_path # change password page (giving an invalid password) diff --git a/spec/features/users/self_registration_spec.rb b/spec/features/users/self_registration_spec.rb index 22689df5a8..eafc101fe5 100644 --- a/spec/features/users/self_registration_spec.rb +++ b/spec/features/users/self_registration_spec.rb @@ -29,7 +29,8 @@ require 'spec_helper' describe 'user self registration', type: :feature, js: true do - let(:admin) { FactoryBot.create :admin, password: 'Test123Test123', password_confirmation: 'Test123Test123' } + let(:admin_password) { 'Test123Test123' } + let(:admin) { FactoryBot.create :admin, password: admin_password, password_confirmation: admin_password } let(:home_page) { Pages::Home.new } context 'with "manual account activation"', @@ -104,7 +105,7 @@ describe 'user self registration', type: :feature, js: true do .to have_content I18n.t(:'account.error_inactive_manual_activation') # activation as admin - login_with admin.login, admin.password + login_with admin.login, admin_password user_page = Pages::Admin::Users::Edit.new(registered_user.id)