Improve handling of emails from addresses with spaces (fixes #2198)

app/models/channel/filter/identify_sender.rb

@@ -143,63 +143,55 @@ module Channel::Filter::IdentifySender
     end
   end
 
-  def self.user_create(data, role_ids = nil)
-    data[:email] += '@local' if !data[:email].match?(/@/)
-    data[:email] = cleanup_email(data[:email])
-    user = User.find_by(email: data[:email]) ||
-           User.find_by(login: data[:email])
+  def self.user_create(attrs, role_ids = nil)
+    populate_attributes!(attrs, role_ids: role_ids)
 
-    # check if firstname or lastname need to be updated
-    if user
-      if user.firstname.blank? && user.lastname.blank?
-        if data[:firstname].present?
-          data[:firstname] = cleanup_name(data[:firstname])
-          user.update!(
-            firstname: data[:firstname]
-          )
-        end
-      end
-      return user
+    if (user = User.find_by('email = :email OR login = :email', attrs))
+      user.update!(attrs.slice(:firstname)) if user.no_name? && attrs[:firstname].present?
+    elsif (user = User.create!(attrs))
+      user.update!(updated_by_id: user.id, created_by_id: user.id)
     end
 
-    # create new user
-    role_ids ||= Role.signup_role_ids
-
-    # fillup
-    %w[firstname lastname].each do |item|
-      if data[item.to_sym].nil?
-        data[item.to_sym] = ''
-      end
-      data[item.to_sym] = cleanup_name(data[item.to_sym])
-    end
-    data[:password]      = ''
-    data[:active]        = true
-    data[:role_ids]      = role_ids
-    data[:updated_by_id] = 1
-    data[:created_by_id] = 1
-
-    user = User.create!(data)
-    user.update!(
-      updated_by_id: user.id,
-      created_by_id: user.id,
-    )
     user
   end
 
-  def self.cleanup_name(string)
-    string.strip!
-    string.delete!('"')
-    string.gsub!(/^'/, '')
-    string.gsub!(/'$/, '')
-    string.gsub!(/.+?\s\(.+?\)$/, '')
-    string
+  def self.populate_attributes!(attrs, **extras)
+    if attrs[:email].match?(/\S\s+\S/)
+      attrs[:preferences] = { mail_delivery_failed: true,
+                              mail_delivery_failed_reason: 'invalid email',
+                              mail_delivery_failed_data: Time.zone.now }
+    end
+
+    attrs.merge!(
+      email: sanitize_email(attrs[:email]),
+      firstname: sanitize_name(attrs[:firstname]),
+      lastname: sanitize_name(attrs[:lastname]),
+      password: '',
+      active: true,
+      role_ids: extras[:role_ids] || Role.signup_role_ids,
+      updated_by_id: 1,
+      created_by_id: 1
+    )
   end
 
-  def self.cleanup_email(string)
+  def self.sanitize_name(string)
+    return '' if string.nil?
+
+    string.strip
+          .delete('"')
+          .gsub(/^'/, '')
+          .gsub(/'$/, '')
+          .gsub(/.+?\s\(.+?\)$/, '')
+  end
+
+  def self.sanitize_email(string)
+    string += '@local' if !string.include?('@')
+
     string.downcase
           .strip
           .delete('"')
-          .sub(/\A'(.*)'\z/, '\1')
+          .sub(/\A'(.*)'\z/, '\1') # see https://github.com/zammad/zammad/issues/2154
+          .gsub(/\s/, '')          # see https://github.com/zammad/zammad/issues/2198
   end
 
 end

app/models/user.rb

@@ -19,7 +19,8 @@ class User < ApplicationModel
   has_many                :authorizations, after_add: :cache_update, after_remove: :cache_update
   belongs_to              :organization,   inverse_of: :members
 
-  before_validation :check_name, :check_email, :check_login, :check_mail_delivery_failed, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier
+  before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier
+  before_validation :check_mail_delivery_failed, on: :update
   before_create   :check_preferences_default, :validate_preferences, :validate_ooo, :domain_based_assignment, :set_locale
   before_update   :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute
   after_create    :avatar_for_email_check
@@ -891,6 +892,10 @@ try to find correct name
     nil
   end
 
+  def no_name?
+    firstname.blank? && lastname.blank?
+  end
+
   private
 
   def check_name
@@ -967,9 +972,8 @@ try to find correct name
   end
 
   def check_mail_delivery_failed
-    return true if !changes || !changes['email']
+    return if email_change.blank?
     preferences.delete(:mail_delivery_failed)
-    true
   end
 
   def ensure_roles

spec/models/channel/email_parser_spec.rb

@@ -50,5 +50,25 @@ RSpec.describe Channel::EmailParser, type: :model do
         end
       end
     end
+
+    # see https://github.com/zammad/zammad/issues/2198
+    context 'when sender address contains spaces (#2198)' do
+      let(:mail_file) { Rails.root.join('test', 'data', 'mail', 'mail071.box') }
+      let(:sender_email) { 'powerquadrantsystem@example.com' }
+
+      it 'removes them before creating a new user' do
+        expect { described_class.new.process({}, raw_mail) }
+          .to change { User.where(email: sender_email).count }.to(1)
+      end
+
+      it 'marks new user email as invalid' do
+        described_class.new.process({}, raw_mail)
+
+        expect(User.find_by(email: sender_email).preferences)
+          .to include('mail_delivery_failed' => true)
+          .and include('mail_delivery_failed_reason' => 'invalid email')
+          .and include('mail_delivery_failed_data' => a_kind_of(ActiveSupport::TimeWithZone))
+      end
+    end
   end
 end

test/data/mail/mail071.box

@@ -0,0 +1,19 @@
+Received: from localhost by xx.xx.io
+	with SpamAssassin (version 3.4.1);
+	Tue, 21 Aug 2018 00:37:30 +0200
+From: "your secret past" <power quadrant system@example.com>
+To: <user@example.com>
+Subject: how've things been?
+Date: Mon, 20 Aug 2018 18:21:59 -0400
+Message-Id: <0.0.164306.jxtmxj281d2xtvlji447768.0@example.com>
+X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on xx.xx.io
+X-Spam-Flag: YES
+X-Spam-Level: ***************
+X-Spam-Status: Yes, score=15.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
+	DKIM_VALID_AU,HTML_FONT_LOW_CONTRAST,HTML_IMAGE_RATIO_04,HTML_MESSAGE,
+	RCVD_IN_BL_SPAMCOP_NET,RCVD_IN_MSPIKE_BL,RCVD_IN_MSPIKE_L5,RCVD_IN_RP_RNBL,
+	RCVD_IN_SBL_CSS,RCVD_IN_SORBS_WEB,RDNS_NONE,SPF_PASS,URIBL_ABUSE_SURBL,
+	URIBL_BLACK,URIBL_DBL_SPAM shortcircuit=no autolearn=no autolearn_force=no
+	version=3.4.1
+MIME-Version: 1.0
+Content-Type: multipart/mixed; boundary="----------=_5B7B42AA.3193F6D3"