From 52f36f4edccc0bb6b20cfadfdadd3c6dd7824a96 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 1 Aug 2017 10:15:37 +0200 Subject: [PATCH] Added postmaster filter validation. Change behaviour of using regular expressions - use explizit regular expressions by prefixing with "regex:your_regexp". --- .../app/controllers/_channel/email.coffee | 14 +- .../app/models/postmaster_filter.coffee | 2 +- app/models/channel/filter/database.rb | 52 +- app/models/postmaster_filter.rb | 22 + test/unit/email_postmaster_test.rb | 519 ++++++++++++++++-- 5 files changed, 546 insertions(+), 63 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_channel/email.coffee b/app/assets/javascripts/app/controllers/_channel/email.coffee index a6855923a..5c92bc279 100644 --- a/app/assets/javascripts/app/controllers/_channel/email.coffee +++ b/app/assets/javascripts/app/controllers/_channel/email.coffee @@ -108,7 +108,7 @@ class App.ChannelEmailFilterEdit extends App.ControllerModal # show errors in form if errors @log 'error', errors - @formValidate( form: e.target, errors: errors ) + @formValidate(form: e.target, errors: errors) return false # disable form @@ -118,8 +118,10 @@ class App.ChannelEmailFilterEdit extends App.ControllerModal object.save( done: => @close() - fail: => - @close() + fail: (settings, details) => + @log 'errors', details + @formEnable(e) + @form.showAlert(details.error_human || details.error || 'Unable to create object!') ) class App.ChannelEmailSignature extends App.Controller @@ -201,7 +203,7 @@ class App.ChannelEmailSignatureEdit extends App.ControllerModal # show errors in form if errors @log 'error', errors - @formValidate( form: e.target, errors: errors ) + @formValidate(form: e.target, errors: errors) return false # disable form @@ -211,8 +213,10 @@ class App.ChannelEmailSignatureEdit extends App.ControllerModal object.save( done: => @close() - fail: => + fail: (settings, details) => + @log 'errors', details @formEnable(e) + @form.showAlert(details.error_human || details.error || 'Unable to create object!') ) class App.ChannelEmailAccountOverview extends App.Controller diff --git a/app/assets/javascripts/app/models/postmaster_filter.coffee b/app/assets/javascripts/app/models/postmaster_filter.coffee index 30c2fa5b3..2ebddcc10 100644 --- a/app/assets/javascripts/app/models/postmaster_filter.coffee +++ b/app/assets/javascripts/app/models/postmaster_filter.coffee @@ -6,7 +6,7 @@ class App.PostmasterFilter extends App.Model @configure_attributes = [ { name: 'name', display: 'Name', tag: 'input', type: 'text', limit: 250, 'null': false }, { name: 'channel', display: 'Channel', type: 'input', readonly: 1 }, - { name: 'match', display: 'Match all of the following', tag: 'postmaster_match' }, + { name: 'match', display: 'Match all of the following', tag: 'postmaster_match', note: 'You can use regular expression by using "regex:your_reg_exp".' }, { name: 'perform', display: 'Perform action of the following', tag: 'postmaster_set' }, { name: 'note', display: 'Note', tag: 'textarea', limit: 250, null: true }, { name: 'updated_at', display: 'Updated', tag: 'datetime', readonly: 1 }, diff --git a/app/models/channel/filter/database.rb b/app/models/channel/filter/database.rb index 822638950..06077abe7 100644 --- a/app/models/channel/filter/database.rb +++ b/app/models/channel/filter/database.rb @@ -14,26 +14,27 @@ module Channel::Filter::Database filter[:match].each { |key, meta| begin next if meta.blank? || meta['value'].blank? + value = mail[ key.downcase.to_sym ] + match_rule = meta['value'] min_one_rule_exists = true - has_matched = false - if mail[ key.downcase.to_sym ].present? && mail[ key.downcase.to_sym ] =~ /#{meta['value']}/i - has_matched = true - end - if has_matched - if meta[:operator] == 'contains not' + if meta[:operator] == 'contains not' + if value.present? && match(value, match_rule, false) all_matches_ok = false + Rails.logger.info " matching #{key.downcase}:'#{value}' on #{match_rule}, but shoud not" + end + elsif meta[:operator] == 'contains' + if value.blank? || !match(value, match_rule, true) + all_matches_ok = false + Rails.logger.info " not matching #{key.downcase}:'#{value}' on #{match_rule}, but should" end - Rails.logger.info " matching #{key.downcase}:'#{mail[ key.downcase.to_sym ]}' on #{meta['value']}" else - if meta[:operator] == 'contains' - all_matches_ok = false - end - Rails.logger.info " not matching #{key.downcase}:'#{mail[ key.downcase.to_sym ]}' on #{meta['value']}" + all_matches_ok = false + Rails.logger.info " Invalid operator in match #{meta.inspect}" end break if !all_matches_ok rescue => e all_matches_ok = false - Rails.logger.error "can't use match rule #{meta['value']} on #{mail[ key.to_sym ]}" + Rails.logger.error "can't use match rule #{match_rule} on #{value}" Rails.logger.error e.inspect end } @@ -49,4 +50,31 @@ module Channel::Filter::Database } end + + def self.match(value, match_rule, _should_match, check_mode = false) + + regexp = false + if match_rule =~ /^(regex:)(.+?)$/ + regexp = true + match_rule = $2 + end + + if regexp == false + match_rule_quoted = Regexp.quote(match_rule).gsub(/\\\*/, '.*') + return true if value =~ /#{match_rule_quoted}/i + return false + end + + begin + return true if value =~ /#{match_rule}/i + return false + rescue => e + message = "Can't use regex '#{match_rule}' on '#{value}': #{e.message}" + Rails.logger.error message + raise message if check_mode == true + end + + false + end + end diff --git a/app/models/postmaster_filter.rb b/app/models/postmaster_filter.rb index f56bf98ea..51ffff1df 100644 --- a/app/models/postmaster_filter.rb +++ b/app/models/postmaster_filter.rb @@ -4,4 +4,26 @@ class PostmasterFilter < ApplicationModel store :perform store :match validates :name, presence: true + + before_create :validate_condition + before_update :validate_condition + + def validate_condition + raise Exceptions::UnprocessableEntity, 'Min. one match rule needed!' if match.blank? + match.each { |_key, meta| + raise Exceptions::UnprocessableEntity, 'operator invalid, ony "contains" and "contains not" is supported' if meta['operator'].blank? || meta['operator'] !~ /^(contains|contains not)$/ + raise Exceptions::UnprocessableEntity, 'value invalid/empty' if meta['value'].blank? + begin + if meta['operator'] == 'contains not' + Channel::Filter::Database.match('test content', meta['value'], false, true) + else + Channel::Filter::Database.match('test content', meta['value'], true, true) + end + rescue => e + raise Exceptions::UnprocessableEntity, e.message + end + } + true + end + end diff --git a/test/unit/email_postmaster_test.rb b/test/unit/email_postmaster_test.rb index afffb22be..3893ddab3 100644 --- a/test/unit/email_postmaster_test.rb +++ b/test/unit/email_postmaster_test.rb @@ -3,6 +3,376 @@ require 'test_helper' class EmailPostmasterTest < ActiveSupport::TestCase + test 'valid/invalid postmaster filter' do + PostmasterFilter.create!( + name: 'not used', + match: { + from: { + operator: 'contains', + value: 'nobody@example.com', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + assert_raises(Exceptions::UnprocessableEntity) { + PostmasterFilter.create!( + name: 'empty filter should not work', + match: {}, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + } + assert_raises(Exceptions::UnprocessableEntity) { + PostmasterFilter.create!( + name: 'empty filter should not work', + match: { + from: { + operator: 'contains', + value: '', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + } + assert_raises(Exceptions::UnprocessableEntity) { + PostmasterFilter.create!( + name: 'invalid regex', + match: { + from: { + operator: 'contains', + value: 'regex:[]', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + } + assert_raises(Exceptions::UnprocessableEntity) { + PostmasterFilter.create!( + name: 'invalid regex', + match: { + from: { + operator: 'contains', + value: 'regex:??', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + } + assert_raises(Exceptions::UnprocessableEntity) { + PostmasterFilter.create!( + name: 'invalid regex', + match: { + from: { + operator: 'contains', + value: 'regex:*', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + } + PostmasterFilter.create!( + name: 'use .*', + match: { + from: { + operator: 'contains', + value: '*', + }, + }, + perform: { + 'X-Zammad-Ticket-priority' => { + value: '3 high', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + end + + test 'process with postmaster filter with regex' do + group_default = Group.lookup(name: 'Users') + group1 = Group.create_if_not_exists( + name: 'Test Group1', + created_by_id: 1, + updated_by_id: 1, + ) + group2 = Group.create_if_not_exists( + name: 'Test Group2', + created_by_id: 1, + updated_by_id: 1, + ) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'used - empty selector', + match: { + from: { + operator: 'contains', + value: 'regex:.*', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group2.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: '1', + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: Some Body +To: Bob +Cc: any@example.com +Subject: some subject - no selector + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + + assert_equal('Test Group2', ticket.group.name) + assert_equal('1 low', ticket.priority.name) + assert_equal('some subject - no selector', ticket.title) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(true, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'used - empty selector', + match: { + from: { + operator: 'contains', + value: '*', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group2.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: '1', + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: Some Body +To: Bob +Cc: any@example.com +Subject: some subject - no selector + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + + assert_equal('Test Group2', ticket.group.name) + assert_equal('1 low', ticket.priority.name) + assert_equal('some subject - no selector', ticket.title) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(true, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'used - empty selector', + match: { + subject: { + operator: 'contains', + value: '*me*', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group2.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: '1', + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: Some Body +To: Bob +Cc: any@example.com +Subject: *me* + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + + assert_equal('Test Group2', ticket.group.name) + assert_equal('1 low', ticket.priority.name) + assert_equal('*me*', ticket.title) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(true, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'used - empty selector', + match: { + subject: { + operator: 'contains not', + value: '*me*', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group2.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: '1', + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: Some Body +To: Bob +Cc: any@example.com +Subject: *mo* + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + + assert_equal('Test Group2', ticket.group.name) + assert_equal('1 low', ticket.priority.name) + assert_equal('*mo*', ticket.title) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(true, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'used - empty selector', + match: { + subject: { + operator: 'contains not', + value: '*me*', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group2.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: '1', + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: Some Body +To: Bob +Cc: any@example.com +Subject: *me* + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + + assert_equal('Users', ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('*me*', ticket.title) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(false, article.internal) + + PostmasterFilter.destroy_all + end + test 'process with postmaster filter' do group_default = Group.lookup(name: 'Users') group1 = Group.create_if_not_exists( @@ -159,51 +529,6 @@ Some Text' PostmasterFilter.destroy_all - PostmasterFilter.create!( - name: 'used - empty selector', - match: { - from: { - operator: 'contains', - value: '', - }, - }, - perform: { - 'X-Zammad-Ticket-group_id' => { - value: group2.id, - }, - 'X-Zammad-Ticket-priority_id' => { - value: '1', - }, - 'x-Zammad-Article-Internal' => { - value: true, - }, - }, - channel: 'email', - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - data = 'From: Some Body -To: Bob -Cc: any@example.com -Subject: some subject - no selector - -Some Text' - - parser = Channel::EmailParser.new - ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) - - assert_equal('Users', ticket.group.name) - assert_equal('2 normal', ticket.priority.name) - assert_equal('some subject - no selector', ticket.title) - - assert_equal('Customer', article.sender.name) - assert_equal('email', article.type.name) - assert_equal(false, article.internal) - - PostmasterFilter.destroy_all - # follow up with create post master filter test PostmasterFilter.create!( name: 'used - empty selector', @@ -477,6 +802,110 @@ Some Text' To: customer@example.com Subject: some subject +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + assert_equal('Users', ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('some subject', ticket.title) + assert_equal('me@example.com', ticket.customer.email) + assert_equal('2 normal', ticket.priority.name) + + assert_equal('Customer', article.sender.name) + assert_equal('email', article.type.name) + assert_equal(false, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'Autoresponder', + match: { + 'auto-submitted' => { + 'operator' => 'contains not', + 'value' => 'auto-generated', + }, + 'to' => { + 'operator' => 'contains', + 'value' => 'customer@example.com', + }, + 'from' => { + 'operator' => 'contains', + 'value' => '@example.com', + } + }, + perform: { + 'x-zammad-article-internal' => { + 'value' => 'true', + }, + 'x-zammad-article-type_id' => { + 'value' => Ticket::Article::Type.find_by(name: 'note').id.to_s, + }, + 'x-zammad-ignore' => { + 'value' => 'false', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: ME Bob +To: customer@example.com +Subject: some subject + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + assert_equal('Users', ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('some subject', ticket.title) + assert_equal('me@example.com', ticket.customer.email) + assert_equal('2 normal', ticket.priority.name) + + assert_equal('Customer', article.sender.name) + assert_equal('note', article.type.name) + assert_equal(true, article.internal) + + PostmasterFilter.destroy_all + PostmasterFilter.create!( + name: 'Autoresponder', + match: { + 'auto-submitted' => { + 'operator' => 'contains', + 'value' => 'auto-generated', + }, + 'to' => { + 'operator' => 'contains', + 'value' => 'customer1@example.com', + }, + 'from' => { + 'operator' => 'contains', + 'value' => '@example.com', + } + }, + perform: { + 'x-zammad-article-internal' => { + 'value' => 'true', + }, + 'x-zammad-article-type_id' => { + 'value' => Ticket::Article::Type.find_by(name: 'note').id.to_s, + }, + 'x-zammad-ignore' => { + 'value' => 'false', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: ME Bob +To: customer@example.com +Subject: some subject + Some Text' parser = Channel::EmailParser.new