From bcce8c51f9c5a9080bfb0af385041c0cfb3d4ea4 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 19 Feb 2020 15:57:32 +0100 Subject: [PATCH] Fixed issue #2904 - Error when changing passwords for users without email. --- app/controllers/users_controller.rb | 93 +++++++++------- app/models/transaction/notification.rb | 2 +- app/models/user_device.rb | 7 ++ lib/notification_factory/mailer.rb | 4 +- spec/lib/notification_factory/mailer_spec.rb | 27 +++++ spec/models/user_device_spec.rb | 27 +++++ spec/requests/user_spec.rb | 105 +++++++++++++++++++ 7 files changed, 225 insertions(+), 40 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5f9cff287..9c77fbc1f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -597,11 +597,16 @@ curl http://localhost/api/v1/users/password_reset -v -u #{login}:#{password} -H result = User.password_reset_new_token(params[:username]) if result && result[:token] - # send mail - user = result[:user] + # unable to send email + if !result[:user] || result[:user].email.blank? + render json: { message: 'failed' }, status: :ok + return + end + + # send password reset emails NotificationFactory::Mailer.notification( template: 'password_reset', - user: user, + user: result[:user], objects: result ) @@ -642,38 +647,48 @@ curl http://localhost/api/v1/users/password_reset_verify -v -u #{login}:#{passwo =end def password_reset_verify - if params[:password] - # check password policy - result = password_policy(params[:password]) - if result != true - render json: { message: 'failed', notice: result }, status: :ok + # check if feature is enabled + raise Exceptions::UnprocessableEntity, 'Feature not enabled!' if !Setting.get('user_lost_password') + raise Exceptions::UnprocessableEntity, 'token param needed!' if params[:token].blank? + + # if no password is given, verify token only + if params[:password].blank? + user = User.by_reset_token(params[:token]) + if user + render json: { message: 'ok', user_login: user.login }, status: :ok return end - - # set new password with token - user = User.password_reset_via_token(params[:token], params[:password]) - - # send mail - if user - NotificationFactory::Mailer.notification( - template: 'password_change', - user: user, - objects: { - user: user, - current_user: current_user, - } - ) - end - - else - user = User.by_reset_token(params[:token]) - end - if user - render json: { message: 'ok', user_login: user.login }, status: :ok - else render json: { message: 'failed' }, status: :ok + return end + + # check password policy + result = password_policy(params[:password]) + if result != true + render json: { message: 'failed', notice: result }, status: :ok + return + end + + # set new password with token + user = User.password_reset_via_token(params[:token], params[:password]) + + # send mail + if !user || user.email.blank? + render json: { message: 'failed' }, status: :ok + return + end + + NotificationFactory::Mailer.notification( + template: 'password_change', + user: user, + objects: { + user: user, + current_user: current_user, + } + ) + + render json: { message: 'ok', user_login: user.login }, status: :ok end =begin @@ -725,14 +740,16 @@ curl http://localhost/api/v1/users/password_change -v -u #{login}:#{password} -H user.update!(password: params[:password_new]) - NotificationFactory::Mailer.notification( - template: 'password_change', - user: user, - objects: { - user: user, - current_user: current_user, - } - ) + if user.email.present? + NotificationFactory::Mailer.notification( + template: 'password_change', + user: user, + objects: { + user: user, + current_user: current_user, + } + ) + end render json: { message: 'ok', user_login: user.login }, status: :ok end diff --git a/app/models/transaction/notification.rb b/app/models/transaction/notification.rb index dcf9bd57b..f7cd2daa7 100644 --- a/app/models/transaction/notification.rb +++ b/app/models/transaction/notification.rb @@ -159,7 +159,7 @@ class Transaction::Notification end # ignore email channel notification and empty emails - if !channels['email'] || !user.email || user.email == '' + if !channels['email'] || user.email.blank? add_recipient_list(ticket, user, used_channels, @item[:type]) next end diff --git a/app/models/user_device.rb b/app/models/user_device.rb index 5785385d7..20dfc8cf2 100644 --- a/app/models/user_device.rb +++ b/app/models/user_device.rb @@ -202,6 +202,11 @@ send user notification about new device or new location for device def notification_send(template) user = User.find(user_id) + if user.email.blank? + Rails.logger.info { "Unable to notification (#{template}) to user_id: #{user.id} be cause of missing email address." } + return false + end + Rails.logger.debug { "Send notification (#{template}) to: #{user.email}" } NotificationFactory::Mailer.notification( @@ -212,6 +217,8 @@ send user notification about new device or new location for device user: user, } ) + + true end =begin diff --git a/lib/notification_factory/mailer.rb b/lib/notification_factory/mailer.rb index b6760839f..480a55d71 100644 --- a/lib/notification_factory/mailer.rb +++ b/lib/notification_factory/mailer.rb @@ -133,6 +133,8 @@ returns =end def self.send(data) + raise Exceptions::UnprocessableEntity, "Unable to send mail to user with id #{data[:recipient][:id]} because there is no email available." if data[:recipient][:email].blank? + sender = Setting.get('notification_sender') Rails.logger.info "Send notification to: #{data[:recipient][:email]} (from:#{sender}/subject:#{data[:subject]})" @@ -146,7 +148,7 @@ returns if channel.blank? Rails.logger.info "Can't find an active 'Email::Notification' channel. Canceling notification sending." - return + return false end channel.deliver( diff --git a/spec/lib/notification_factory/mailer_spec.rb b/spec/lib/notification_factory/mailer_spec.rb index 30ee233b6..901162540 100644 --- a/spec/lib/notification_factory/mailer_spec.rb +++ b/spec/lib/notification_factory/mailer_spec.rb @@ -90,4 +90,31 @@ RSpec.describe NotificationFactory::Mailer do end end end + + describe '#send' do + subject(:result) do + described_class.send( + recipient: user, + subject: 'some subject', + body: 'some body', + ) + end + + context 'recipient with email address' do + let(:user) { create(:agent_user, email: 'somebody@example.com') } + + it 'returns a Mail::Message' do + expect( result ).to be_kind_of(Mail::Message) + end + end + + context 'recipient without email address' do + let(:user) { create(:agent_user, email: '') } + + it 'raises Exceptions::UnprocessableEntity' do + expect { result }.to raise_error(Exceptions::UnprocessableEntity) + end + end + end + end diff --git a/spec/models/user_device_spec.rb b/spec/models/user_device_spec.rb index 2f27e3223..48b0d25c4 100644 --- a/spec/models/user_device_spec.rb +++ b/spec/models/user_device_spec.rb @@ -219,4 +219,31 @@ RSpec.describe UserDevice, type: :model do end end end + + describe '#notification_send' do + let(:user_device) { described_class.add(user_agent, ip, agent.id, fingerprint, type) } + let(:user_agent) { 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.107 Safari/537.36' } + let(:ip) { '91.115.248.231' } + let(:fingerprint) { 'fingerprint1234' } + let(:type) { 'session' } + + context 'user with email address' do + let(:agent) { create(:agent_user, email: 'somebody@example.com') } + + it 'returns true' do + expect(user_device.notification_send('user_device_new_location')) + .to eq(true) + end + end + + context 'user without email address' do + let(:agent) { create(:agent_user, email: '') } + + it 'returns false' do + expect(user_device.notification_send('user_device_new_location')) + .to eq(false) + end + end + end + end diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index e132e9893..20109950f 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -1036,5 +1036,110 @@ RSpec.describe 'User', type: :request, searchindex: true do result.collect! { |v| v['id'] } expect(result).to eq([user1.id, user2.id]) end + + context 'does password reset send work' do + let(:user) { create(:customer_user, login: 'somebody', email: 'somebody@example.com') } + + context 'for user without email address' do + let(:user) { create(:customer_user, login: 'somebody', email: '') } + + it 'return failed' do + post '/api/v1/users/password_reset', params: { username: user.login }, as: :json + + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response['message']).to eq('failed') + end + end + + context 'for user with email address' do + it 'return ok' do + post '/api/v1/users/password_reset', params: { username: user.login }, as: :json + + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response['message']).to eq('ok') + end + end + + context 'for user with email address but disabled feature' do + before { Setting.set('user_lost_password', false) } + + it 'raise 422' do + post '/api/v1/users/password_reset', params: { username: user.login }, as: :json + + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['error']).to be_truthy + expect(json_response['error']).to eq('Feature not enabled!') + end + end + end + + context 'does password reset by token work' do + let(:user) { create(:customer_user, login: 'somebody', email: 'somebody@example.com') } + let(:token) { create(:token, action: 'PasswordReset', user_id: user.id) } + + context 'for user without email address' do + let(:user) { create(:customer_user, login: 'somebody', email: '') } + + it 'return failed' do + 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) + expect(json_response['message']).to eq('failed') + end + end + + 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 + + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response['message']).to eq('ok') + end + end + + context 'for user with email address but disabled feature' do + before { Setting.set('user_lost_password', false) } + + it 'raise 422' do + post '/api/v1/users/password_reset_verify', params: { username: user.login, token: token.name, password: 'Test1234#.' }, as: :json + + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response['error']).to be_truthy + expect(json_response['error']).to eq('Feature not enabled!') + end + end + end + + context 'password change' do + let(:user) { create(:customer_user, login: 'somebody', email: 'somebody@example.com', password: 'Test1234#.') } + + before { authenticated_as(user, login: 'somebody', password: 'Test1234#.') } + + context 'user without email address' do + let(:user) { create(:customer_user, login: 'somebody', email: '', password: 'Test1234#.') } + + it 'return ok' do + 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) + expect(json_response['message']).to eq('ok') + end + end + + 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 + + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response['message']).to eq('ok') + end + end + end end end