From ec74d56b7730aaa805bdcb4faae7a2fd5ca83604 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Thu, 30 Aug 2018 10:39:50 +0200 Subject: [PATCH] Improve handling of emails from addresses with spaces (fixes #2198) --- app/models/channel/filter/identify_sender.rb | 84 +++++++++----------- app/models/user.rb | 10 ++- spec/models/channel/email_parser_spec.rb | 20 +++++ test/data/mail/mail071.box | 19 +++++ 4 files changed, 84 insertions(+), 49 deletions(-) create mode 100644 test/data/mail/mail071.box diff --git a/app/models/channel/filter/identify_sender.rb b/app/models/channel/filter/identify_sender.rb index cd57c173e..69b4bf9e6 100644 --- a/app/models/channel/filter/identify_sender.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 4139703e9..f1fd4d44c 100644 --- a/app/models/user.rb +++ b/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 diff --git a/spec/models/channel/email_parser_spec.rb b/spec/models/channel/email_parser_spec.rb index 346bf99a0..78fd86898 100644 --- a/spec/models/channel/email_parser_spec.rb +++ b/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 diff --git a/test/data/mail/mail071.box b/test/data/mail/mail071.box new file mode 100644 index 000000000..d364a99d3 --- /dev/null +++ b/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" +To: +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"