Fixed issue #2904 - Error when changing passwords for users without email.

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
+      # unable to send email
-      user = result[:user]
+      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
+    # check if feature is enabled
-      result = password_policy(params[:password])
+    raise Exceptions::UnprocessableEntity, 'Feature not enabled!' if !Setting.get('user_lost_password')
-      if result != true
+    raise Exceptions::UnprocessableEntity, 'token param needed!' if params[:token].blank?
-        render json: { message: 'failed', notice: result }, status: :ok
+
+    # 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(
+    if user.email.present?
-      template: 'password_change',
+      NotificationFactory::Mailer.notification(
-      user:     user,
+        template: 'password_change',
-      objects:  {
+        user:     user,
-        user:         user,
+        objects:  {
-        current_user: current_user,
+          user:         user,
-      }
+          current_user: current_user,
-    )
+        }
+      )
+    end
 
     render json: { message: 'ok', user_login: user.login }, status: :ok
   end

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

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

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(

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

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

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